Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Bloom filters on unsigned integer columns in Parquet tables #9770

Merged
merged 8 commits into from
Apr 2, 2024
Merged

Add support for Bloom filters on unsigned integer columns in Parquet tables #9770

merged 8 commits into from
Apr 2, 2024

Conversation

progval
Copy link
Contributor

@progval progval commented Mar 24, 2024 •
edited
Loading

Which issue does this PR close?

Closes #9769.

Rationale for this change

What changes are included in this PR?

Also fixed prune_int $bits _scalar_fun_and_eq (on signed integers) to run the query it should have been running.

Are these changes tested?

They are not, but the code is straightforward so I don't think tests would be useful.

Are there any user-facing changes?

It should be invisible other than a performance improvement.

@github-actions github-actions bot added the core Core DataFusion crate label Mar 24, 2024
@alamb alamb mentioned this pull request Mar 24, 2024
Closed
7 tasks
alamb
alamb reviewed Mar 24, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much @progval 🙏

Can we please add tests for this code (so that we don't accidentally break it in some future refactoring?)

@alamb alamb mentioned this pull request Mar 25, 2024
Closed
6 tasks
@alamb
Copy link
Contributor

alamb commented Mar 30, 2024

Thank you very much @progval 🙏

Can we please add tests for this code (so that we don't accidentally break it in some future refactoring?)

Hi @progval -- do you think you will have time to add some tests briefly

I noticed that #9778 only adds tests for the signed (Int8/Int16/etc variants). I'll go get that PR ready to merge per our discussion as maybe you wrote it to add tests for this one 🤔

@progval
Copy link
Contributor Author

progval commented Mar 30, 2024

as maybe you wrote it to add tests for this one 🤔

yes that's what I had in mind

@alamb
Copy link
Contributor

alamb commented Mar 31, 2024

Ok, #9778 has now been merged. Sorry for the delay

@progval
Copy link
Contributor Author

progval commented Mar 31, 2024 •
edited
Loading

Tests added. There is some code in common with #9888 (to build the test data) but they shouldn't merge-conflict

@progval
Copy link
Contributor Author

progval commented Mar 31, 2024 •
edited
Loading

but they shouldn't merge-conflict

They did, but I just fixed it.

@alamb alamb mentioned this pull request Apr 1, 2024
Open
alamb
alamb approved these changes Apr 1, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @progval -- this looks great to me

The test failures appear to be due to #9891

I took the liberty of merging up to main to get the CI to pass

alamb
alamb reviewed Apr 1, 2024
View reviewed changes
datafusion/core/tests/parquet/row_group_pruning.rs
@@ -452,6 +452,144 @@ int_tests!(16, correct_bloom_filters: false);
int_tests!(32, correct_bloom_filters: true);
int_tests!(64, correct_bloom_filters: true);

// $bits: number of bits of the integer to test (8, 16, 32, 64)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't realize UInt8 and UInt16 were also affected. 💯 for testing

@alamb alamb mentioned this pull request Apr 1, 2024
Open
@comphead comphead merged commit e9c5dc9 into apache:main Apr 2, 2024
24 checks passed
@alamb alamb mentioned this pull request Apr 2, 2024
Closed
@progval progval mentioned this pull request Apr 18, 2024
Closed
10 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb alamb approved these changes

Assignees
No one assigned
Labels
core Core DataFusion crate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Bloom filters on unsigned integer columns in Parquet tables
3 participants
@progval @alamb @comphead