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

make page filter public #6523

Merged
merged 4 commits into from
Jun 8, 2023
Merged

make page filter public #6523

merged 4 commits into from
Jun 8, 2023

Conversation

jiacai2050
Copy link
Contributor

@jiacai2050 jiacai2050 commented Jun 2, 2023 •
edited
Loading

Which issue does this PR close?

Closes #.

Rationale for this change

We at CeresDB are using a customized parquet reader based on parquet-rs for some historical reason, in order to reuse those page prune functions from datafusion, those interface should be public.

What changes are included in this PR?

  • Public page_filter related API.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Jun 2, 2023
@jiacai2050 jiacai2050 changed the title Page filter WIP: Page filter Jun 2, 2023
@jiacai2050 jiacai2050 changed the title WIP: Page filter WIP: make page filter public Jun 2, 2023
alamb
alamb reviewed Jun 2, 2023
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.

This is a fine change in my mind -- @jiacai2050 can you comment about why you would like to have this API public ? I didn't see any rationale or a linked ticket?

Also if you want to use this API long term I suggest we implement an end to end test (maybe in datafusion/core/tests/parquet somewhere) for it so that it isn't broken accidentally in some future refactor

@jiacai2050
Copy link
Contributor Author

@alamb Thanks, I'm still testing this changeset in CeresDB, I will add more context once I'm done.

Also if you want to use this API long term I suggest we implement an end to end test (maybe in datafusion/core/tests/parquet somewhere) for it so that it isn't broken accidentally in some future refactor

Agreed!

@alamb
Copy link
Contributor

alamb commented Jun 5, 2023

This PR still says "WIP" in the title so I am not sure if you think it is ready to merge @jiacai2050 -- or if you plan to work on an end to end test before doing so?

@alamb
Copy link
Contributor

alamb commented Jun 5, 2023

I believe if you merge up to latest main branch the clippy failure has been resolved.

@alamb alamb marked this pull request as draft June 5, 2023 19:15
@jiacai2050 jiacai2050 marked this pull request as ready for review June 7, 2023 02:09
@jiacai2050 jiacai2050 changed the title WIP: make page filter public make page filter public Jun 7, 2023
@jiacai2050 jiacai2050 requested a review from alamb June 7, 2023 02:13
@alamb
Copy link
Contributor

alamb commented Jun 7, 2023

It appears that cargo doc wants documentation for public structures, so the structures that are now pub need documentation to pass CI

@jiacai2050 jiacai2050 requested a review from alamb June 8, 2023 08:32
@alamb alamb merged commit 4973c7c into apache:main Jun 8, 2023
@alamb
Copy link
Contributor

alamb commented Jun 8, 2023

THanks @jiacai2050

@jiacai2050 jiacai2050 deleted the page-filter branch June 8, 2023 13:50
jayzhan211 pushed a commit to jayzhan211/datafusion that referenced this pull request Jun 12, 2023
@jiacai2050 @jayzhan211
make page filter public (apache#6523)
c2dac1b 
* make page_filter public

* make parquet public

* fix CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alamb alamb Awaiting requested review from alamb

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.
2 participants
@jiacai2050 @alamb