-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
PARQUET-34: Extend Contains support to all ColumnFilterPredicate types #1370
PARQUET-34: Extend Contains support to all ColumnFilterPredicate types #1370
Conversation
add(" set = true;\n"); | ||
|
||
add(" break;\n"); | ||
add(" setResult(true);\n return;\n"); |
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 simplified the existing ValueInspector logic a bit for In
/NotIn
here. Previously it was:
@Override
public void update(int value) {
boolean set = false;
for (int i : target) {
if(comparator.compare(value, i) != 0) {
setResult(true);
set = true;
break;
}
}
if (!set) setResult(false);
}
};
}
Now it's:
@Override
public void update(int value) {
for (int i : target) {
if(comparator.compare(value, i) == 0 ) {
setResult(true);
return;
}
}
setResult(false);
}
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.
+1
Sorry for the delay. cc @gszadovszky
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.
Looks good to me! I have just one nit comment.
|| underlying instanceof SetColumnFilterPredicate | ||
&& ((SetColumnFilterPredicate) underlying) | ||
.getValues() | ||
.contains(null)) { |
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.
nit: I know it means the same logically, but I think it is more readable to wrap in a parenthesis since the instanceof
and the related cast are tightly related.
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.
makes sense! updated
Follows up on this discussion from #1328 to fast-follow by expanding
Contains
predicate support from onlyEq<T>
to allColumnFilterPredicate<T>
types.Jira
them in the PR title. For example, "PARQUET-1234: My Parquet PR"
the ASF 3rd Party License Policy.
Tests
Commits
from "How to write a good git commit message":
Style
mvn spotless:apply -Pvector-plugins
Documentation