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.

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 in and not in predicates #357

Open
wants to merge 8 commits into
base: master
from

Conversation

@jun-he
Copy link
Contributor

commented Aug 7, 2019

Implements #39 .

@jun-he

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

I took a different approach than the previous incomplete PR (Netflix/iceberg#81).
Instead of adding CollectionLiteral and CollectionPredicate etc, I added a new Set field to predicate to support in and notIn.
I think this approach is better than the previous one and the cost of adding an additional set field is quite low.

@rdblue Could you please have a look? Thank you.

@rdblue

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Nice work, @jun-he! I think this is looking good, but there are a few areas to fix. Thanks for working on this!

@jun-he jun-he changed the title Jun/in and not in predicates in and not in predicates Aug 10, 2019
@jun-he jun-he changed the title in and not in predicates Add in and not in predicates Aug 10, 2019
jun-he added 4 commits Aug 7, 2019
1. add various checks
2. add implicit conversion for IN and NOT_IN during construction and binding
3. add LiteralSet and optimize to directly check if containing literal wrapped value
4. add BoundSetPredicate class
5. add additional unit tests
@jun-he jun-he force-pushed the jun-he:jun/in-and-notIn-predicates branch from 6b93f84 to 0387a3b Aug 13, 2019
Also revert commit 0387a3b as it is not needed any more.
@jun-he

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2019

@rdblue I have addressed the review comments. Could you take a look when you get a chance?

@jun-he

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2019

@rdblue Addressed all the comments. Thanks.

@jun-he

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2019

@rdblue I have addressed the review comments. Could you take a look when you get a chance?
Thanks.

@rdblue

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

@jun-he, thanks for the update. I'll review soon, but my current focus is getting ready for a Java release. Thanks!

@rdblue rdblue added this to the Java 0.8.0 Release milestone Oct 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.