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

Use TupleDomain#intersect for List<TupleDomain> in DynamicFilterService #10700

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

raunaqmorarka
Copy link
Member

@raunaqmorarka raunaqmorarka commented Jan 20, 2022

Allow single element list in TupleDomain#intersect to make it's usage easier.
Using the direct API for intersecting lists can be more efficient than
intersecting two elements at a time.

@findepi
Copy link
Member

findepi commented Jan 20, 2022

@raunaqmorarka please add to TestTupleDomain

Allowed single element and empty list in TupleDomain#intersect
to make it's usage easier.
Using the direct API for intersecting lists can be more efficient than
intersecting two elements at a time.
@raunaqmorarka
Copy link
Member Author

@raunaqmorarka please add to TestTupleDomain

added tests in TestTupleDomain


assertEquals(TupleDomain.intersect(ImmutableList.of()), all());
assertEquals(TupleDomain.intersect(ImmutableList.of(tupleDomain1)), tupleDomain1);
assertEquals(TupleDomain.intersect(ImmutableList.of(tupleDomain1, tupleDomain2)), expectedTupleDomain);
Copy link
Member

Choose a reason for hiding this comment

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

yikes, we had no tests here for TupleDomain.intersect(List)... thanks for adding them!

@sopel39 sopel39 merged commit ca0946b into trinodb:master Jan 24, 2022
@sopel39
Copy link
Member

sopel39 commented Jan 24, 2022

This probably does not need RN

@github-actions github-actions bot added this to the 369 milestone Jan 24, 2022
@raunaqmorarka raunaqmorarka deleted the list-intersect branch January 24, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants