-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Add projectStrict for Dates and Timestamps #283
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
Merged
rdblue
merged 21 commits into
apache:master
from
moulimukherjee:implement-strict-projection
Jul 26, 2019
Merged
Changes from all commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
dc485bb
Add projectStrict for Dates and Timestamps
mouli-stripe f533ecd
Add tests for Timestamps
mouli-stripe 8343037
Adding more tests with lower and upper bounds
mouli-stripe c3ad7e3
Fixing tests for TimestampsProjections
mouli-stripe fae0b36
Adding tests for Dates Projection
mouli-stripe c3620e5
Refactor tests
mouli-stripe 304321f
Formatting
mouli-stripe b90edff
Removing duplicate test
mouli-stripe 3a39123
Using generics instead of Integer for the truncateIntegerStrict and t…
mouli-stripe 712ce22
Correcting upper bound test values
mouli-stripe f976325
Fix Residual Evaluator
mouli-stripe a82faaf
Refactoring logic
mouli-stripe c8155f8
Addressing review comments
mouli-stripe 07ae999
Removing unnecessary changes to ResidualEvaluator
mouli-stripe 33df0d3
Adding explicit case for EQ
mouli-stripe ad1d7a7
Add examples
mouli-stripe 40e1471
Simplifying the LT and GT predicates
mouli-stripe ba3262e
Getting rid of white space
mouli-stripe 0722e22
Making all the predicates either LT_EQ or GT_EQ
mouli-stripe 30f3594
Adding equal and notEqual tests for all
mouli-stripe dc15fbb
Fixing style error
mouli-stripe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,12 +19,9 @@ | |
|
|
||
| package org.apache.iceberg.expressions; | ||
|
|
||
| import com.google.common.collect.Iterables; | ||
| import com.google.common.collect.Lists; | ||
| import java.io.Serializable; | ||
| import java.util.Comparator; | ||
| import java.util.List; | ||
| import java.util.Objects; | ||
| import org.apache.iceberg.PartitionField; | ||
| import org.apache.iceberg.PartitionSpec; | ||
| import org.apache.iceberg.StructLike; | ||
|
|
@@ -200,42 +197,66 @@ public <T> Expression notEq(BoundReference<T> ref, Literal<T> lit) { | |
| @Override | ||
| @SuppressWarnings("unchecked") | ||
| public <T> Expression predicate(BoundPredicate<T> pred) { | ||
| // Get the strict projection of this predicate in partition data, then use it to determine | ||
| // whether to return the original predicate. The strict projection returns true iff the | ||
| // original predicate would have returned true, so the predicate can be eliminated if the | ||
| // strict projection evaluates to true. | ||
| /** | ||
| * Get the strict projection and inclusive projection of this predicate in partition data, | ||
| * then use them to determine whether to return the original predicate. The strict projection | ||
| * returns true iff the original predicate would have returned true, so the predicate can be | ||
| * eliminated if the strict projection evaluates to true. Similarly the inclusive projection | ||
| * returns false iff the original predicate would have returned false, so the predicate can | ||
| * also be eliminated if the inclusive projection evaluates to false. | ||
| */ | ||
|
|
||
| // | ||
| // If there is no strict projection or if it evaluates to false, then return the predicate. | ||
| List<PartitionField> parts = spec.getFieldsBySourceId(pred.ref().fieldId()); | ||
| if (parts == null) { | ||
| return pred; // not associated inclusive a partition field, can't be evaluated | ||
| } | ||
|
|
||
| List<UnboundPredicate<?>> strictProjections = Lists.transform(parts, | ||
| part -> ((Transform<T, ?>) part.transform()).projectStrict(part.name(), pred)); | ||
| for (PartitionField part : parts) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like how you restructured this to simply return |
||
|
|
||
| if (Iterables.all(strictProjections, Objects::isNull)) { | ||
| // if there are no strict projections, the predicate must be in the residual | ||
| return pred; | ||
| } | ||
| // checking the strict projection | ||
| UnboundPredicate<?> strictProjection = ((Transform<T, ?>) part.transform()).projectStrict(part.name(), pred); | ||
| Expression strictResult = null; | ||
|
|
||
| if (strictProjection != null) { | ||
| Expression bound = strictProjection.bind(spec.partitionType(), caseSensitive); | ||
| if (bound instanceof BoundPredicate) { | ||
| strictResult = super.predicate((BoundPredicate<?>) bound); | ||
| } else { | ||
| // if the result is not a predicate, then it must be a constant like alwaysTrue or alwaysFalse | ||
| strictResult = bound; | ||
|
moulimukherjee marked this conversation as resolved.
|
||
| } | ||
| } | ||
|
|
||
| Expression result = Expressions.alwaysFalse(); | ||
| for (UnboundPredicate<?> strictProjection : strictProjections) { | ||
| if (strictProjection == null) { | ||
| continue; | ||
| if (strictResult != null && strictResult.op() == Expression.Operation.TRUE) { | ||
| // If strict is true, returning true | ||
| return Expressions.alwaysTrue(); | ||
| } | ||
|
|
||
| Expression bound = strictProjection.bind(spec.partitionType(), caseSensitive); | ||
| if (bound instanceof BoundPredicate) { | ||
| // evaluate the bound predicate, which will return alwaysTrue or alwaysFalse | ||
| result = Expressions.or(result, super.predicate((BoundPredicate<?>) bound)); | ||
| } else { | ||
| // update the result expression with the non-predicate residual (e.g. alwaysTrue) | ||
| result = Expressions.or(result, bound); | ||
| // checking the inclusive projection | ||
| UnboundPredicate<?> inclusiveProjection = ((Transform<T, ?>) part.transform()).project(part.name(), pred); | ||
| Expression inclusiveResult = null; | ||
| if (inclusiveProjection != null) { | ||
| Expression boundInclusive = inclusiveProjection.bind(spec.partitionType(), caseSensitive); | ||
| if (boundInclusive instanceof BoundPredicate) { | ||
| // using predicate method specific to inclusive | ||
| inclusiveResult = super.predicate((BoundPredicate<?>) boundInclusive); | ||
| } else { | ||
| // if the result is not a predicate, then it must be a constant like alwaysTrue or alwaysFalse | ||
| inclusiveResult = boundInclusive; | ||
| } | ||
| } | ||
|
|
||
| if (inclusiveResult != null && inclusiveResult.op() == Expression.Operation.FALSE) { | ||
| // If inclusive is false, returning false | ||
| return Expressions.alwaysFalse(); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| return result; | ||
| // neither strict not inclusive predicate was conclusive, returning the original pred | ||
| return pred; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
added null check