Parquet: Set parquet bloom filter config with compatible column name #5435
Conversation
|
Hi, @huaxingao @rdblue @kbendick . Could you help to review this when you are free? Thanks a lot. |
| .forEach( | ||
| (columnPath, value) -> { | ||
| // check the column exists and support bloom filter | ||
| Types.NestedField field = schema.caseInsensitiveFindField(columnPath); |
There was a problem hiding this comment.
Maybe we should add this check at the begining, filtering all invalid keys and making field names compatible before pushing these props down to writer. What do you think?
There was a problem hiding this comment.
I think it should do in setting/updating properties. Actually, we already do some simple checkers in our company. I could submit a PR for this and waiting for @rdblue opinion.
| "Invalid bloom filter column configuration: field {} is of type '{}' which cannot " | ||
| + "benefit from a bloom filter. This column configuration will be ignored.", | ||
| columnPath, | ||
| fieldType); |
There was a problem hiding this comment.
I don't think this is worth checking.
There was a problem hiding this comment.
Hi @rdblue, thanks for your time. The most end user doesn't understand the parquet bloom filter implementation details. The log warning here could give some users more information about their settings. Just as @zhongyujiang mentioned, maybe we should add some checking when setting/updating table properties.
| @@ -951,6 +968,72 @@ public void testStructFieldNotEq() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testStructFieldWithSpecialCharacterLt() { | |||
There was a problem hiding this comment.
@huaxingao, can you review these tests?
There was a problem hiding this comment.
@rdblue These tests look good. Thanks for pinging me.
|
Thanks @rdblue @kbendick @zhongyujiang @huaxingao for your time to review. Comments have been addressed. Pls take another look when you are free. |
|
@huaxingao can you take another look and push forward ? thanks |
|
LGTM |
|
cc @rdblue Could you please take another look when you have a moment? |
|
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
This PR improves the config setting for the parquet bloom filter.