-
Notifications
You must be signed in to change notification settings - Fork 388
Add case-sensitive support for equality deletes in DeleteFilter #1930
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.
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
Add case-sensitive support for equality deletes in DeleteFilter #1930
Conversation
…nsitive. This change adds optional case-sensitive matching when binding equality delete predicates. - Updated build_equality_delete_predicate to handle case-sensitive option. - Added unit tests for case-sensitive equality deletes.
8fc60d3 to
e711f8b
Compare
|
@liurenjie1024 @Xuanwo Could you please help review this PR? Thank you very much! |
liurenjie1024
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @slfan1989 for this pr, generally LGTM, just one minor change.
| pub name_mapping: Option<Arc<NameMapping>>, | ||
|
|
||
| /// Whether this scan task should treat column names as case-sensitive when binding predicates. | ||
| pub case_sensitive: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be set according TableScan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@liurenjie1024 Thank you very much for reviewing the code! Yes, the case_sensitive flag should indeed be set according to the TableScan. It represents a query-level property that needs to flow down through the plan context and manifest contexts into each FileScanTask, so that DeleteFilter can correctly bind equality delete predicates with the intended case-sensitivity.
The overall call chain is:
TableScan → PlanContext → ManifestFileContext → ManifestEntryContext → FileScanTask
liurenjie1024
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @slfan1989 for this fix!
@liurenjie1024 Thanks a lot for reviewing the code! |
Which issue does this PR close?
What changes are included in this PR?
This change adds optional case-sensitive matching when binding equality delete predicates.
Are these changes tested?
Added a new unit test.