You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@parquet.apache.org by "clairemcginty (via GitHub)" <gi...@apache.org> on 2024/07/04 16:19:09 UTC
[PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
clairemcginty opened a new pull request, #2941:
URL: https://github.com/apache/parquet-java/pull/2941
This should be the final subtask for the Contains() predicate work. It adds support for logical inversion -- `not(contains(...))`.
Also, it fixes a small bug with the composition of mixed Contains predicates.
Make sure you have checked _all_ steps below.
### Issue
- [x] My PR addresses the following [GitHub issues](https://github.com/apache/parquet-java/issues) and references
them in the PR title. For example, "GH-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-34 (should I make a new Github issue for this?)
- In case you are adding a dependency, check if the license complies with
the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
### Tests
- [x] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [x] My commits all reference GitHub issues in their subject lines. In addition, my commits follow the guidelines
from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including GitHub issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Style
- [ ] My contribution adheres to the code style guidelines and Spotless passes.
- To apply the necessary changes, run `mvn spotless:apply -Pvector-plugins`
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#issuecomment-2249468292
@gszadovszky Do you want to take a look?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#discussion_r1665897688
##########
parquet-column/src/main/java/org/apache/parquet/filter2/recordlevel/IncrementallyUpdatedFilterPredicate.java:
##########
@@ -145,6 +146,83 @@ public boolean accept(Visitor visitor) {
}
}
+ /**
+ * A ValueInspector implementation that keeps state for one or more delegate inspectors.
+ */
+ abstract static class DelegatingValueInspector extends ValueInspector {
Review Comment:
I created this because I thought the generated `IncrementallyUpdatedFilterPredicateBuilder` was getting complex in terms of LOC -- all the `Contains` ValueInspectors were overriding `public void update(int|float|binary|double|... value)` as well as `reset()`, and it was getting hard to reason about. However.... this is probably a bit less efficient, since the delegate ValueInspectors are accessed via iterator loops rather than directly invoking `left`/`right`. Let me know what you think, I'm happy to change it back to the way it was
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#discussion_r1667871679
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java:
##########
@@ -345,11 +374,14 @@ public <R> R accept(Visitor<R> visitor) {
}
/**
- * Applies a filtering Vistitor to the Contains predicate, traversing any composed And or Or clauses,
- * and finally delegating to the underlying ColumnFilterPredicate.
+ * Applies a filtering Visitor to the Contains predicate, traversing any composed And or Or clauses,
+ * and finally delegating to the underlying column predicate.
*/
public abstract <R> R filter(
- Visitor<R> visitor, BiFunction<R, R, R> andBehavior, BiFunction<R, R, R> orBehavior);
+ Visitor<R> visitor,
+ BiFunction<R, R, R> andBehavior,
+ BiFunction<R, R, R> orBehavior,
+ Function<R, R> notBehavior);
Review Comment:
IIUC, all kinds of filters return all rows if `notBehavior` is found, right?
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -371,7 +371,11 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
@Override
public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
- return contains.filter(this, IndexIterator::intersection, IndexIterator::union);
+ return contains.filter(
Review Comment:
Yes, we cannot make such subtraction unless it is a single-value page which has only `bar`.
##########
parquet-column/src/main/java/org/apache/parquet/filter2/recordlevel/IncrementallyUpdatedFilterPredicate.java:
##########
@@ -145,6 +146,83 @@ public boolean accept(Visitor visitor) {
}
}
+ /**
+ * A ValueInspector implementation that keeps state for one or more delegate inspectors.
+ */
+ abstract static class DelegatingValueInspector extends ValueInspector {
Review Comment:
`IncrementallyUpdatedFilterPredicateGenerator.java` is pretty difficult to read and review. Since this is only applied to `ContainsPredicate` and does not affect others, I'm fine to make this change.
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/statisticslevel/StatisticsFilter.java:
##########
@@ -214,7 +214,7 @@ public <T extends Comparable<T>> Boolean visit(NotIn<T> notIn) {
@Override
public <T extends Comparable<T>> Boolean visit(Contains<T> contains) {
- return contains.filter(this, (l, r) -> l || r, (l, r) -> l && r);
+ return contains.filter(this, (l, r) -> l || r, (l, r) -> l && r, b -> BLOCK_MIGHT_MATCH);
Review Comment:
```suggestion
return contains.filter(this, (l, r) -> l || r, (l, r) -> l && r, v -> BLOCK_MIGHT_MATCH);
```
Just want to be consistent with `BloomFilterImpl` and `DictionaryFilter`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#discussion_r1666001736
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -371,7 +371,11 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
@Override
public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
- return contains.filter(this, IndexIterator::intersection, IndexIterator::union);
+ return contains.filter(
Review Comment:
My initial idea was to implement `IndexIterator::subtract` for `not(contains())` -- for example, `not(contains(eq("bar")))` would return something like `IndexIterator.all(getPageCount()).subtract(visit(eq("bar"))`. But we can't rely on the individual predicate implementations to return exact row ranges -- for example, `NotIn` [always returns IndexIterator.all](https://github.com/apache/parquet-java/blob/c26624fd8fc196f3d65edb0bcb0460a03cd163fd/parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java#L369). So I don't think it's possible to safely subtract any row ranges here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#discussion_r1665896250
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/ContainsRewriter.java:
##########
@@ -118,15 +118,18 @@ public FilterPredicate visit(And and) {
} else if (and.getRight() instanceof Contains) {
right = and.getRight();
} else {
- return and;
+ right = and.getRight();
}
- if (left instanceof Contains) {
- if (!(right instanceof Contains)) {
- throw new UnsupportedOperationException(
- "Contains predicates cannot be composed with non-Contains predicates");
- }
+ // If two Contains predicates refer to the same column, optimize by combining into a single predicate
Review Comment:
This fixes a bug with the way Contains predicates were composed -- previously it would throw an error if you tried to do something like `and(contains(binaryColumn("foo", ...), contains(binaryColumn("bar"), ...))` because the columns didn't match. however, that is still a valid predicate -- it's just that they should not be attempted to be combined into a single `Contains` predicate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#issuecomment-2257320102
Merged. Thanks @clairemcginty and @gszadovszky!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org
Re: [PR] PARQUET-34: implement not() for Contains predicate [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #2941:
URL: https://github.com/apache/parquet-java/pull/2941#discussion_r1677940560
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java:
##########
@@ -345,11 +374,14 @@ public <R> R accept(Visitor<R> visitor) {
}
/**
- * Applies a filtering Vistitor to the Contains predicate, traversing any composed And or Or clauses,
- * and finally delegating to the underlying ColumnFilterPredicate.
+ * Applies a filtering Visitor to the Contains predicate, traversing any composed And or Or clauses,
+ * and finally delegating to the underlying column predicate.
*/
public abstract <R> R filter(
- Visitor<R> visitor, BiFunction<R, R, R> andBehavior, BiFunction<R, R, R> orBehavior);
+ Visitor<R> visitor,
+ BiFunction<R, R, R> andBehavior,
+ BiFunction<R, R, R> orBehavior,
+ Function<R, R> notBehavior);
Review Comment:
Yes, for example `NotIn` 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@parquet.apache.org
For additional commands, e-mail: issues-help@parquet.apache.org