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/04/23 19:43:55 UTC
[PR] PARQUET-34: Add #contains FilterPredicate for Array columns [parquet-mr]
clairemcginty opened a new pull request, #1328:
URL: https://github.com/apache/parquet-mr/pull/1328
Proposal to add new FilterPredicates, `contains` and `doesNotContain`, that can be applied to List types, and check if the specified element is present among the repeated values:
```java
FilterPredicate predicate = FilterApi.contains(
FilterApi.arrayColumn(FilterApi.binaryColumn("phoneNumbers.phone.kind")),
Binary.fromString("cell")
)
```
The filtering logic is largely based on existing Eq/In predicates to apply filtering at the page/rowgroup level using statistics/dictionaries, with a specialized implementation in `IncrementallyUpdatedFilterPredicateBuilder` to do individual record-level filtering.
### Jira
- [x] My PR addresses the following [Parquet Jira](https://issues.apache.org/jira/browse/PARQUET/) issues and references
them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-34
- 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 Jira 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 Jira 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
- [x] 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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1579571960
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Yes, I agree that it worths making the function composable. Perhaps we can define all the necessary API (with implementation of the most common ones) in the PR and implementations of all other kinds of inputs can be split into separate PRs. Let's not hurry for the coming release. It still takes time to stabilize.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1606996592
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,20 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
Review Comment:
Updated the PR with implementations for each. I implemented them as new `PrimitiveIterator.OfInt`s that zip together the component left & right hand sides, deduplicating/intersecting/merging as needed. Implementing custom iterators is kind of error prone though... if this adds too much complexity I can always save it for a followup PR, or just materialize the iterators into `List`s which are a lot easier to work with 😅
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1606767983
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,20 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
Review Comment:
Makes sense. I'll look into doing an intersection/union operation here. Or otherwise just fall back to not filtering and returning `IndexIterator.all`.
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#issuecomment-2146417119
Thanks @gszadovszky for the detail review! I'll take another pass shortly to be familiar with the latest change and then merge it if no concern.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582080399
##########
pom.xml:
##########
@@ -603,6 +603,46 @@
<exclude>org.apache.parquet.io.api.Binary#get2BytesLittleEndian()</exclude>
<exclude>org.apache.parquet.schema.LogicalTypeAnnotation$Float16LogicalTypeAnnotation#accept(org.apache.parquet.schema.LogicalTypeAnnotation$LogicalTypeAnnotationVisitor)</exclude>
+ <exclude>org.apache.parquet.filter2.predicate.Operators$Contains</exclude>
Review Comment:
I ended up removing ArrayColumn and rebasing onto the latest master branch, and the exclusions were no longer needed 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1604376286
##########
parquet-generator/src/main/java/org/apache/parquet/filter2/IncrementallyUpdatedFilterPredicateGenerator.java:
##########
@@ -122,6 +127,11 @@ public void run() throws IOException {
}
addVisitEnd();
+ addContainsBegin();
+ addVisitBegin("Contains");
+ addContainsCase();
Review Comment:
Nothing to do with a `addContainsEnd()`? Perhaps adding a noop one for better readability
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/ContainsRewriter.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.filter2.predicate;
+
+import java.util.Objects;
+import org.apache.parquet.filter2.predicate.FilterPredicate.Visitor;
+import org.apache.parquet.filter2.predicate.Operators.And;
+import org.apache.parquet.filter2.predicate.Operators.Contains;
+import org.apache.parquet.filter2.predicate.Operators.Eq;
+import org.apache.parquet.filter2.predicate.Operators.Gt;
+import org.apache.parquet.filter2.predicate.Operators.GtEq;
+import org.apache.parquet.filter2.predicate.Operators.In;
+import org.apache.parquet.filter2.predicate.Operators.LogicalNotUserDefined;
+import org.apache.parquet.filter2.predicate.Operators.Lt;
+import org.apache.parquet.filter2.predicate.Operators.LtEq;
+import org.apache.parquet.filter2.predicate.Operators.Not;
+import org.apache.parquet.filter2.predicate.Operators.NotEq;
+import org.apache.parquet.filter2.predicate.Operators.NotIn;
+import org.apache.parquet.filter2.predicate.Operators.Or;
+import org.apache.parquet.filter2.predicate.Operators.UserDefined;
+
+/**
+ * Recursively rewrites Contains predicates composed using And or Or into a single Contains predicate
+ * containing all predicate assertions.
+ *
+ * This is a performance optimization, as all composed Contains sub-predicates must share the same column, and
+ * can therefore can be applied efficiently as a single predicate pass.
+ */
+public final class ContainsRewriter implements Visitor<FilterPredicate> {
+ private static final ContainsRewriter INSTANCE = new ContainsRewriter();
+
+ public static FilterPredicate rewrite(FilterPredicate pred) {
+ Objects.requireNonNull(pred, "pred cannot be null");
+ return pred.accept(INSTANCE);
+ }
+
+ private ContainsRewriter() {}
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Eq<T> eq) {
+ return eq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(NotEq<T> notEq) {
+ return notEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Lt<T> lt) {
+ return lt;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(LtEq<T> ltEq) {
+ return ltEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Gt<T> gt) {
+ return gt;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(GtEq<T> gtEq) {
+ return gtEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(In<T> in) {
+ return in;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(NotIn<T> notIn) {
+ return notIn;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Contains<T> contains) {
+ return contains;
+ }
+
+ @Override
+ public FilterPredicate visit(And and) {
+ final FilterPredicate left;
+ if (and.getLeft() instanceof And) {
+ left = visit((And) and.getLeft());
+ } else if (and.getLeft() instanceof Or) {
+ left = visit((Or) and.getLeft());
+ } else if (and.getLeft() instanceof Contains) {
+ left = and.getLeft();
+ } else {
+ return and;
+ }
+
+ final FilterPredicate right;
+ if (and.getRight() instanceof And) {
+ right = visit((And) and.getRight());
+ } else if (and.getRight() instanceof Or) {
+ right = visit((Or) and.getRight());
+ } else if (and.getRight() instanceof Contains) {
+ right = and.getRight();
+ } else {
+ return and;
+ }
+
+ if (left instanceof Contains) {
+ if (!(right instanceof Contains)) {
+ throw new UnsupportedOperationException(
+ "Contains predicates cannot be composed with non-Contains predicates");
+ }
+ return ((Contains) left).and(right);
+ } else {
+ return and;
+ }
+ }
+
+ @Override
+ public FilterPredicate visit(Or or) {
+ final FilterPredicate left;
+ if (or.getLeft() instanceof And) {
+ left = visit((And) or.getLeft());
+ } else if (or.getLeft() instanceof Or) {
+ left = visit((Or) or.getLeft());
+ } else if (or.getLeft() instanceof Contains) {
+ left = or.getLeft();
+ } else {
+ return or;
+ }
+
+ final FilterPredicate right;
+ if (or.getRight() instanceof And) {
+ right = visit((And) or.getRight());
+ } else if (or.getRight() instanceof Or) {
+ right = visit((Or) or.getRight());
+ } else if (or.getRight() instanceof Contains) {
+ right = or.getRight();
+ } else {
+ return or;
+ }
+
+ if (left instanceof Contains) {
+ if (!(right instanceof Contains)) {
+ throw new UnsupportedOperationException(
+ "Contains predicates cannot be composed with non-Contains predicates");
+ }
+ return ((Contains) left).or(right);
+ } else {
+ return or;
+ }
+ }
+
+ @Override
+ public FilterPredicate visit(Not not) {
+ return not;
Review Comment:
I'm not sure if we can safely throw here as `LogicalInverseRewriter` is (and should be) called before any other rewriter.
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,20 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
Review Comment:
CMIW, throwing exception here might prohibit `contains` predicate to be used to filter parquet files with page index available.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2082148570
> > BTW, should we postpone this feature to 1.15.0 release? We can always release new version as needed and I can volunteer to be the release manager once this is ready.
>
> I agree with @wgtmac. Let's create smaller PRs and make improvements then release when we feel it stable. During the development, we may advertise this feature so others may start experimenting on it and give feedback before actually releasing it. A too early release might make later improvements harder because we need to be backward compatible.
Sounds good to me! this PR might take another week or two to get right. It would also be nice to release support for operations like Array#size at the same time, so pushing it to 0.15 would give us time to do that 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2120335645
I tried adding a test case to `TestRecordLevelFilters` to test `contains(eq(null))`. My expectation was that if you have an array schema with an optional element type, this should return true if the array contains one or more null elements. However, I don't think this is possible to make work--I set a debugger on `ValueInspector#update` and `ValueInspector#updateNull`. and `ValueInspector#update` is only invoked for non-null elements, and `ValueInspector#updateNull` is only invoked if the entire array is null, which isn't exactly what we want, either.
So based on my current understanding, I don't think we can support a `contains(eq(null))` predicate, and we can probably add a precondition check to the `Contains` constructor against null elements. Wdyt @wgtmac ?
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#issuecomment-2124038889
@wgtmac, completely agree to have more people reviewing this. Thanks for pinging.
I'll try to take a look this week.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2081961577
BTW, should we postpone this feature to 1.15.0 release? We can always release new version as needed and I can volunteer to be the release manager once this is ready.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1589036357
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
ok, I think I mostly got this working, supporting just Contains Eq (+ composition using And/Or) to start. hopefully this is closer to what you had in mind!
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1576808087
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java:
##########
@@ -119,6 +119,16 @@ public <T extends Comparable<T>> Boolean visit(Operators.GtEq<T> gtEq) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Operators.Contains<T> contains) {
+ return BLOCK_MIGHT_MATCH;
Review Comment:
I was testing this out with the BloomFilter Visitor and couldn't produce a test Parquet file with a populated bloom filter for a List field (I tried strings, ints, and longs as element types). I guess this is expected? Therefore, I didn't override the visit() implementation 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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1624642780
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java:
##########
@@ -55,6 +55,111 @@ static PrimitiveIterator.OfInt rangeTranslate(int from, int to, IntUnaryOperator
return new IndexIterator(from, to + 1, i -> true, translator);
}
+ static PrimitiveIterator.OfInt intersection(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ if (!hasNext()) {
+ throw new NoSuchElementException();
+ }
+ int result = next;
+ next = fetchNext();
+ return result;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return next != -1;
+ }
+
+ private int fetchNext() {
+ if (!lhs.hasNext() || !rhs.hasNext()) {
+ return -1;
+ }
+
+ // Since we know both iterators are in sorted order, we can iterate linearly through until
+ // we find the next value that belongs to both iterators, or terminate if none exist
+ int nextL = lhs.next();
+ int nextR = rhs.next();
+ while (nextL < nextR && lhs.hasNext()) {
+ nextL = lhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
+ while (nextR < nextL && rhs.hasNext()) {
+ nextR = rhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
Review Comment:
ah I see what you mean and replicated this in a test! I think I need to use a broader while loop. Refactoring now.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2117481105
> This overall LGTM! Thanks @clairemcginty for working on this and adding exhaustive test!
great, I'm glad this implementation looks ok! I have a few more tests that I'd like to add around null handling + behavior of the `Contains` predicate on map types (I think that they should just work, but I haven't tried it out yet...). Will try to add those + address PR comments on Monday or Tuesday next week 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1606991187
##########
parquet-generator/src/main/java/org/apache/parquet/filter2/IncrementallyUpdatedFilterPredicateGenerator.java:
##########
@@ -122,6 +127,11 @@ public void run() throws IOException {
}
addVisitEnd();
+ addContainsBegin();
+ addVisitBegin("Contains");
+ addContainsCase();
Review Comment:
Added!
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1589038273
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,20 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
Review Comment:
I need to think about the behavior here one -- `visit(And and)` and `visit(Or or) ` in `ColumnIndexBuilder` throw an UnsupportedOperationException, so I just copied that behavior for composed Contains predicates. But I'm thinking we can actually support this by taking the union/intersection of the `PrimitiveIterator.OfInt`, kind of similar to how `ColumnIndexFilter` [works](https://github.com/apache/parquet-mr/blob/apache-parquet-1.14.0-rc0/parquet-column/src/main/java/org/apache/parquet/internal/filter2/columnindex/ColumnIndexFilter.java#L193-L209).
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582079957
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java:
##########
@@ -119,6 +119,16 @@ public <T extends Comparable<T>> Boolean visit(Operators.GtEq<T> gtEq) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Operators.Contains<T> contains) {
+ return BLOCK_MIGHT_MATCH;
Review Comment:
that worked! thanks 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582197892
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
> It's not a standard SQL function, but I've seen it in SQL extension languages such as [BigQuery Standard SQL](https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#in_operators), and I've gotten several requests to support this by users of the [Scio](https://github.com/spotify/scio) Parquet library!
>
> that's a good point about making this composable, I think it would be more efficient to do `CONTAINS(a or b)` than `CONTAINS(a) or CONTAINS(b)`. What do you think about supporting `lt/gt` in addition to `eq`-based Contains? for example, `CONTAINS(eq(a) OR gt(b))` ? It would make this PR a lot more complex but I'm happy to try. We could probably re-use a lot of the existing filter code for `eq`, `lt/gt`, etc...
I took a second look, it seems that you were mentioning `IN` expression which we already have: https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java#L209-L258
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1581166004
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Actually, I think I need some clarification on the expected behavior of And/Or for an array field...
Say we have the following records, each containing a repeated int field with the following values:
```
Record1:
repeated_int_field: [2, 3, 4, 5]
Record2:
repeated_int_field: [1, 7]
```
Given the predicate `contains(and(gt(3), lt(6)))`, should we return:
1. Only Record1, because it contains **at least one element that satisfies both predicates** gt(3) and lt(6), or
2. Both Record1 and Record2, because Record2 contains **elements that satisfy each predicate separately** (7 matches `gt(3)` but not `lt(6)`, and 1 satisfies `lt(6)` but not `gt(3)`?
I think I would assume 2 🤔 but not totally sure.
cc @gszadovszky as well :)
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1579573018
##########
pom.xml:
##########
@@ -603,6 +603,46 @@
<exclude>org.apache.parquet.io.api.Binary#get2BytesLittleEndian()</exclude>
<exclude>org.apache.parquet.schema.LogicalTypeAnnotation$Float16LogicalTypeAnnotation#accept(org.apache.parquet.schema.LogicalTypeAnnotation$LogicalTypeAnnotationVisitor)</exclude>
+ <exclude>org.apache.parquet.filter2.predicate.Operators$Contains</exclude>
Review Comment:
Is it possible to avoid these exclusions?
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1613060962
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/ContainsRewriter.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.filter2.predicate;
+
+import java.util.Objects;
+import org.apache.parquet.filter2.predicate.FilterPredicate.Visitor;
+import org.apache.parquet.filter2.predicate.Operators.And;
+import org.apache.parquet.filter2.predicate.Operators.Contains;
+import org.apache.parquet.filter2.predicate.Operators.Eq;
+import org.apache.parquet.filter2.predicate.Operators.Gt;
+import org.apache.parquet.filter2.predicate.Operators.GtEq;
+import org.apache.parquet.filter2.predicate.Operators.In;
+import org.apache.parquet.filter2.predicate.Operators.LogicalNotUserDefined;
+import org.apache.parquet.filter2.predicate.Operators.Lt;
+import org.apache.parquet.filter2.predicate.Operators.LtEq;
+import org.apache.parquet.filter2.predicate.Operators.Not;
+import org.apache.parquet.filter2.predicate.Operators.NotEq;
+import org.apache.parquet.filter2.predicate.Operators.NotIn;
+import org.apache.parquet.filter2.predicate.Operators.Or;
+import org.apache.parquet.filter2.predicate.Operators.UserDefined;
+
+/**
+ * Recursively rewrites Contains predicates composed using And or Or into a single Contains predicate
+ * containing all predicate assertions.
+ *
+ * This is a performance optimization, as all composed Contains sub-predicates must share the same column, and
+ * can therefore can be applied efficiently as a single predicate pass.
Review Comment:
nit: extra "can" (not sure which one :smile:)
##########
parquet-column/src/test/java/org/apache/parquet/filter2/predicate/TestFilterApiMethods.java:
##########
@@ -91,13 +93,46 @@ public void testFilterPredicateCreation() {
assertEquals(ColumnPath.get("x", "y", "z"), ((Gt) gt).getColumn().getColumnPath());
}
+ @Test
+ public void testInvalidContainsCreation() {
+ FilterPredicate pred;
+ try {
+ pred = contains(eq(binColumn, null));
+ fail("Contains predicate with null element value should fail");
+ } catch (IllegalArgumentException e) {
+ assertEquals("Contains predicate does not support null element value", e.getMessage());
+ }
Review Comment:
nit: you may use [Assert.assertThrows](https://junit.org/junit4/javadoc/4.13/org/junit/Assert.html#assertThrows(java.lang.String,%20java.lang.Class,%20org.junit.function.ThrowingRunnable))
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/PhoneBookWriter.java:
##########
@@ -54,6 +56,12 @@ public class PhoneBookWriter {
+ " optional binary kind (UTF8);\n"
+ " }\n"
+ " }\n"
+ + " optional group accounts {\n"
Review Comment:
nit: Let's use the `MAP` logical type according to the [spec](https://github.com/apache/parquet-format/blob/master/LogicalTypes.md#maps).
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,102 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
+ return contains.filter(
+ this,
+ // For Contains.And, compute the intersection of the int ranges
+ (l, r) -> new PrimitiveIterator.OfInt() {
+ private Integer next = null;
+
+ @Override
+ public int nextInt() {
+ int result = next;
+ next = null;
+ return result;
+ }
Review Comment:
I don't think this implementation follows the contract of an iterator. Iterators do not enforce the user to call `hasNext` before calling `next`. If you sure about the size, you may just call one `next` after another. It may throw a `NoSuchElementException` if there are no more values.
The trick is to calculate the next value beforehand and check the existence of that value at `hasNext` and calculate the "next-next" value at `next` before returning the pre-calculated next value. (Hope it makes sense :smile:)
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +258,10 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>> Contains<T> contains(Eq<T> pred) {
Review Comment:
Sounds good, but keep in mind that this one and the follow ups shall be released together otherwise we'll have API compatibility issues.
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,102 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
+ return contains.filter(
+ this,
+ // For Contains.And, compute the intersection of the int ranges
+ (l, r) -> new PrimitiveIterator.OfInt() {
+ private Integer next = null;
+
+ @Override
+ public int nextInt() {
+ int result = next;
+ next = null;
+ return result;
+ }
Review Comment:
See [IndexIterator](https://github.com/apache/parquet-java/blob/master/parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java) as an example. You may even put static methods there for `intersection` and `union` and it will be more readable here.
I would not use boxing/unboxing here only to have a additional `null` value. Indices here can only be non-negative so you may use `-1` to mark the case of no more values.
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/TestRecordLevelFilters.java:
##########
@@ -20,6 +20,7 @@
import static org.apache.parquet.filter2.predicate.FilterApi.and;
import static org.apache.parquet.filter2.predicate.FilterApi.binaryColumn;
+import static org.apache.parquet.filter2.predicate.FilterApi.contains;
Review Comment:
During checking the code I've found that this record level testing class does not seem to be correct. In several places (just like in your code) the method `PhoneBookWriter.readFile` is used which calls [PhoneBookWriter.createReader](https://github.com/apache/parquet-java/blob/240ab0daa005e9cd9dcc9d3df716cd321761b307/parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/PhoneBookWriter.java#L335-L339) to create the reader instance. The issue is, by default all the filtering (statistics/dictionary/column index) are working. As a result, we cannot be sure that the read results we are getting are really filtered only by the record level filter.
I know this is not your fault but could you please try using a properly set up reader for this test?
```java
// ...
Configuration conf = new Configuration();
GroupWriteSupport.setSchema(schema, conf);
return ParquetReader.builder(new GroupReadSupport(), file)
.withConf(conf)
.withFilter(filter)
.withAllocator(allocator)
.useBloomFilter(false)
.useDictionaryFilter(false)
.useStatsFilter(false)
.useColumnIndexFilter(false)
.build();
```
If unrelated errors may occur, let's handle this separately, but I want to avoid not catching a potential issue in your code by leaving this test as is.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1606991656
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/ContainsRewriter.java:
##########
@@ -0,0 +1,185 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.filter2.predicate;
+
+import java.util.Objects;
+import org.apache.parquet.filter2.predicate.FilterPredicate.Visitor;
+import org.apache.parquet.filter2.predicate.Operators.And;
+import org.apache.parquet.filter2.predicate.Operators.Contains;
+import org.apache.parquet.filter2.predicate.Operators.Eq;
+import org.apache.parquet.filter2.predicate.Operators.Gt;
+import org.apache.parquet.filter2.predicate.Operators.GtEq;
+import org.apache.parquet.filter2.predicate.Operators.In;
+import org.apache.parquet.filter2.predicate.Operators.LogicalNotUserDefined;
+import org.apache.parquet.filter2.predicate.Operators.Lt;
+import org.apache.parquet.filter2.predicate.Operators.LtEq;
+import org.apache.parquet.filter2.predicate.Operators.Not;
+import org.apache.parquet.filter2.predicate.Operators.NotEq;
+import org.apache.parquet.filter2.predicate.Operators.NotIn;
+import org.apache.parquet.filter2.predicate.Operators.Or;
+import org.apache.parquet.filter2.predicate.Operators.UserDefined;
+
+/**
+ * Recursively rewrites Contains predicates composed using And or Or into a single Contains predicate
+ * containing all predicate assertions.
+ *
+ * This is a performance optimization, as all composed Contains sub-predicates must share the same column, and
+ * can therefore can be applied efficiently as a single predicate pass.
+ */
+public final class ContainsRewriter implements Visitor<FilterPredicate> {
+ private static final ContainsRewriter INSTANCE = new ContainsRewriter();
+
+ public static FilterPredicate rewrite(FilterPredicate pred) {
+ Objects.requireNonNull(pred, "pred cannot be null");
+ return pred.accept(INSTANCE);
+ }
+
+ private ContainsRewriter() {}
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Eq<T> eq) {
+ return eq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(NotEq<T> notEq) {
+ return notEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Lt<T> lt) {
+ return lt;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(LtEq<T> ltEq) {
+ return ltEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Gt<T> gt) {
+ return gt;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(GtEq<T> gtEq) {
+ return gtEq;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(In<T> in) {
+ return in;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(NotIn<T> notIn) {
+ return notIn;
+ }
+
+ @Override
+ public <T extends Comparable<T>> FilterPredicate visit(Contains<T> contains) {
+ return contains;
+ }
+
+ @Override
+ public FilterPredicate visit(And and) {
+ final FilterPredicate left;
+ if (and.getLeft() instanceof And) {
+ left = visit((And) and.getLeft());
+ } else if (and.getLeft() instanceof Or) {
+ left = visit((Or) and.getLeft());
+ } else if (and.getLeft() instanceof Contains) {
+ left = and.getLeft();
+ } else {
+ return and;
+ }
+
+ final FilterPredicate right;
+ if (and.getRight() instanceof And) {
+ right = visit((And) and.getRight());
+ } else if (and.getRight() instanceof Or) {
+ right = visit((Or) and.getRight());
+ } else if (and.getRight() instanceof Contains) {
+ right = and.getRight();
+ } else {
+ return and;
+ }
+
+ if (left instanceof Contains) {
+ if (!(right instanceof Contains)) {
+ throw new UnsupportedOperationException(
+ "Contains predicates cannot be composed with non-Contains predicates");
+ }
+ return ((Contains) left).and(right);
+ } else {
+ return and;
+ }
+ }
+
+ @Override
+ public FilterPredicate visit(Or or) {
+ final FilterPredicate left;
+ if (or.getLeft() instanceof And) {
+ left = visit((And) or.getLeft());
+ } else if (or.getLeft() instanceof Or) {
+ left = visit((Or) or.getLeft());
+ } else if (or.getLeft() instanceof Contains) {
+ left = or.getLeft();
+ } else {
+ return or;
+ }
+
+ final FilterPredicate right;
+ if (or.getRight() instanceof And) {
+ right = visit((And) or.getRight());
+ } else if (or.getRight() instanceof Or) {
+ right = visit((Or) or.getRight());
+ } else if (or.getRight() instanceof Contains) {
+ right = or.getRight();
+ } else {
+ return or;
+ }
+
+ if (left instanceof Contains) {
+ if (!(right instanceof Contains)) {
+ throw new UnsupportedOperationException(
+ "Contains predicates cannot be composed with non-Contains predicates");
+ }
+ return ((Contains) left).or(right);
+ } else {
+ return or;
+ }
+ }
+
+ @Override
+ public FilterPredicate visit(Not not) {
+ return not;
Review Comment:
I think you're right, or at least, all the existing tests passed when I added an exception thrown here 👍
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/TestRecordLevelFilters.java:
##########
@@ -181,6 +187,84 @@ public void testInFilter() throws Exception {
assert (found.size() == 102);
}
+ @Test
Review Comment:
Todo: add tests for a `contains(eq(null))` filter
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1624537559
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java:
##########
@@ -55,6 +55,105 @@ static PrimitiveIterator.OfInt rangeTranslate(int from, int to, IntUnaryOperator
return new IndexIterator(from, to + 1, i -> true, translator);
}
+ static PrimitiveIterator.OfInt intersection(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ int result = next;
Review Comment:
thanks for catching that! Updated both.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582264899
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
ahh, ok. So we’d have a single Contains predicate class that can support one or more sub-predicated, and the Rewriter would inspect any Ands or Ors to see see if the left/right side is an instance of Contains… If so, we rewrite it into a single `Contains` predicate containing both clauses?
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
ahh, ok. So we’d have a single `Contains` predicate class that can support one or more sub-predicated, and the Rewriter would inspect any Ands or Ors to see see if the left/right side is an instance of Contains… If so, we rewrite it into a single `Contains` predicate containing both clauses?
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2075107608
Thanks for the new feature! I will try to take a look soon and make it to the 1.14.0 release.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1581723304
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
I would expect the option 1. Option 2 looks more like contains(or(gt(3),lt(6))). The case would be more complex if we add NOT expr to it. If there is any confusion like this, what about only supporting eq and ne for now? More complicated expressions need more research and inputs from other experts.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1579309150
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
It's not a standard SQL function, but I've seen it in SQL extension languages such as [BigQuery Standard SQL](https://cloud.google.com/bigquery/docs/reference/standard-sql/functions-and-operators#in_operators), and I've gotten several requests to support this by users of the [Scio](https://github.com/spotify/scio) Parquet library!
that's a good point about making this composable, I think it would be more efficient to do `CONTAINS(a or b)` than `CONTAINS(a) or CONTAINS(b)`. What do you think about supporting `lt/gt` in addition to `eq`-based Contains? for example, `CONTAINS(eq(a) OR gt(b))` ? It would make this PR a lot more complex but I'm happy to try. We could probably re-use a lot of the existing filter code for `eq`, `lt/gt`, etc...
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2082043989
> BTW, should we postpone this feature to 1.15.0 release? We can always release new version as needed and I can volunteer to be the release manager once this is ready.
I agree with @wgtmac. Let's create smaller PRs and make improvements then release when we feel it stable. During the development, we may advertise this feature so others may start experimenting on it and give feedback before actually releasing it. A too early release might make later improvements harder because we need to be backward compatible.
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1608679199
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +258,10 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>> Contains<T> contains(Eq<T> pred) {
Review Comment:
In a fast-follow-up PR we can just replace this signature with:
```java
public static <T extends Comparable<T>> Contains<T> contains(ColumnFilterPredicate<T> pred) {
...
}
```
and remove the `SupportsContains` annotation. I did a few tests of `Contains(lt)` and `Contains(gt)` and everything is working as expected 👍
--
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: commits-unsubscribe@parquet.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] PARQUET-34: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582080330
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Ok, I've pushed my changes to support composable contains predicates! example usage:
```java
FilterApi.containsOr(
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 5555555555L),
FilterApi.containsOr(
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), -10000000L), // Won't be matched
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 2222222222L)));
```
I left out implementing DoesNotContain/ContainsNotEq for now to keep the PR simpler to parse. Let me know if the API looks ok 👍 Then I can fill out the rest of the implementations/make the unit tests more thorough.
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Ok, I've pushed my changes to support composable contains predicates! example usage:
```java
FilterApi.containsOr(
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 5555555555L),
FilterApi.containsOr(
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), -10000000L),
FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 2222222222L)));
```
I left out implementing DoesNotContain/ContainsNotEq for now to keep the PR simpler to parse. Let me know if the API looks ok 👍 Then I can fill out the rest of the implementations/make the unit tests more thorough.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1578917312
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
BTW, now these new functions only accept single value. Does it make sense to support variable number of values? I also see some databases support CONTAINS(a OR b) and CONTAINS(a AND b). They can be composited by logical operators but the cost would be high compared to support them natively,
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java:
##########
@@ -119,6 +119,16 @@ public <T extends Comparable<T>> Boolean visit(Operators.GtEq<T> gtEq) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Operators.Contains<T> contains) {
+ return BLOCK_MIGHT_MATCH;
Review Comment:
BloomFilters are created for primitive columns when dictionary encoding is disabled. Would you try that again with dictionary encoding disabled?
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
It seems that Contains/DoesNotContain is not a standard SQL function. In which case they are used?
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -91,6 +95,11 @@ public static BinaryColumn binaryColumn(String columnPath) {
return new BinaryColumn(ColumnPath.fromDotString(columnPath));
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> ArrayColumn<T> arrayColumn(
Review Comment:
Is it possible to reuse this for map key or value column as well?
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1581043500
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java:
##########
@@ -315,6 +321,66 @@ public <R> R accept(Visitor<R> visitor) {
}
}
+ public abstract static class ArrayContainsFilterPredicate<T extends Comparable<T>>
Review Comment:
sounds good! I think I'll have to refactor this API design a bit to make Array filters composable, as @wgtmac suggested above -- will try to push changes late today or Monday 🙏
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2111610919
Sorry for the delay. I will try to finish another pass by the end of this week.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1579598449
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Sounds great. I would love to get predicates for array size in there, too, but I think that one is going to be a bit more complex (we might need to update Statistics to reflect array sizes?)... that one can wait for a separate PR.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1581777941
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
That sounds good! 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1577922105
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
Thinking to the future, it could eventually be useful to support Array FilterPredicates like "repeated field X contains an int field greater than Y". We could refactor this API to be written like:
```java
FilterApi.contains(
FilterApi.eq(
FilterApi.arrayColumn(FilterApi.intColumn("repeated_int_field")),
100
)
)
```
& we could eventually support `FilterApi.contains(FilterApi.lt(...))`, `FilterApi.contains(FilterApi.gt(...))`, etc...
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1589039313
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/TestRecordLevelFilters.java:
##########
@@ -181,6 +187,84 @@ public void testInFilter() throws Exception {
assert (found.size() == 102);
}
+ @Test
Review Comment:
Todo: add tests for a `contains(eq(null))` filter
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1613495538
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java:
##########
@@ -368,6 +369,102 @@ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(NotIn<T> notIn) {
return IndexIterator.all(getPageCount());
}
+ @Override
+ public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
+ return contains.filter(
+ this,
+ // For Contains.And, compute the intersection of the int ranges
+ (l, r) -> new PrimitiveIterator.OfInt() {
+ private Integer next = null;
+
+ @Override
+ public int nextInt() {
+ int result = next;
+ next = null;
+ return result;
+ }
Review Comment:
makes sense! I had put some of the next-loading logic into `hasNext()` because as far as I could tell, this iterator would be used via the `forEachRemaining()` method, which IIRC does sequentially call `hasNext()/next()`... but it does make this a bit unstable 😅 I can rewrite it and move into static methods in `IndexIterator`.
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1613497125
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +258,10 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>> Contains<T> contains(Eq<T> pred) {
Review Comment:
understood -- I can commit to working on the follow-up PR right away 👍
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1613623187
##########
parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/TestRecordLevelFilters.java:
##########
@@ -20,6 +20,7 @@
import static org.apache.parquet.filter2.predicate.FilterApi.and;
import static org.apache.parquet.filter2.predicate.FilterApi.binaryColumn;
+import static org.apache.parquet.filter2.predicate.FilterApi.contains;
Review Comment:
Ha, I did notice that -- I'd been refactoring StatisticsFilter, made a mistake, and was confused why `TestRecordLevelFilter` suddenly started failing.
updated! All tests continued to pass after disabling the other filters.
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1624573384
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java:
##########
@@ -55,6 +55,111 @@ static PrimitiveIterator.OfInt rangeTranslate(int from, int to, IntUnaryOperator
return new IndexIterator(from, to + 1, i -> true, translator);
}
+ static PrimitiveIterator.OfInt intersection(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ if (!hasNext()) {
+ throw new NoSuchElementException();
+ }
+ int result = next;
+ next = fetchNext();
+ return result;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return next != -1;
+ }
+
+ private int fetchNext() {
+ if (!lhs.hasNext() || !rhs.hasNext()) {
+ return -1;
+ }
+
+ // Since we know both iterators are in sorted order, we can iterate linearly through until
+ // we find the next value that belongs to both iterators, or terminate if none exist
+ int nextL = lhs.next();
+ int nextR = rhs.next();
+ while (nextL < nextR && lhs.hasNext()) {
+ nextL = lhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
+ while (nextR < nextL && rhs.hasNext()) {
+ nextR = rhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
Review Comment:
It seems that an outer loop is missing that would run till the end of the shorter iterator. Example:
```
lhs: 1, 3, 5, 7
rhs: 2, 4, 6, 7
```
After these lines at the first invocation of the method `nextL` will be `3` and `nextR` will be `4` and `-1` is about the be returned while `7` is a common element in both.
(It also means that the unit test for this one is not complete...)
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582198726
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
> Ok, I've pushed my changes to support composable contains predicates! example usage:
>
> ```java
> FilterApi.containsOr(
> FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 5555555555L),
> FilterApi.containsOr(
> FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), -10000000L),
> FilterApi.containsEq(longColumn("phoneNumbers.phone.number"), 2222222222L)));
> ```
>
> I left out implementing DoesNotContain/ContainsNotEq for now to keep the PR simpler to parse. Let me know if the API looks ok 👍 Then I can fill out the rest of the implementations/make the unit tests more thorough.
Sorry I didn't make it clear. I thought we could still leverage [existing composite expression](https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java#L293-L326) and combine `CONTAINS(x)` sub-expressions under the same level of `AND` or `OR` expression during rewrite process like https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/compat/FilterCompat.java#L79. It would be harder to maintain the code if introduce new specialized composite expression like `containsOr`.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582264899
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
ahh, ok. So the rewriter would inspect the runtime classes of the `left` and `right` predicates of an `And` or `Or`, and if they match the new Contains* expressions, we rewrite it into a single `Contains` predicate containing both clauses?
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582476292
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
I would expect so, but it seems requiring a lot more work. We can support the simplest case at this point.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#issuecomment-2079033981
As [requested on the dev@parquet ML](https://lists.apache.org/thread/cmlmo4nsdhdzq3sfdb18010jmw54z6s2), I'll wait for this PR before starting the releasing process of 1.14.0.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1582264899
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/FilterApi.java:
##########
@@ -257,6 +266,16 @@ public static <T extends Comparable<T>, C extends Column<T> & SupportsEqNotEq> N
return new NotIn<>(column, values);
}
+ public static <T extends Comparable<T>, C extends Column<T> & SupportsContains> Contains<T> contains(
Review Comment:
ahh, ok. So we’d have a single public `Contains` predicate class that’s registered with the Visitor API, that can support one or more sub-predicates… and the Rewriter would inspect any Ands or Ors to see see if the left/right side is an instance of Contains… If so, we rewrite it into a single `Contains` predicate containing both clauses?
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#issuecomment-2145151247
hey @gszadovszky! all your requested changes have been addressed - anything else that's missing?
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328#discussion_r1624452491
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java:
##########
@@ -55,6 +55,105 @@ static PrimitiveIterator.OfInt rangeTranslate(int from, int to, IntUnaryOperator
return new IndexIterator(from, to + 1, i -> true, translator);
}
+ static PrimitiveIterator.OfInt intersection(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ int result = next;
Review Comment:
Sorry for bothering with the iterator contract again. It should throw `NoSuchElementException` if there are no more elements (instead of returning `-1`).
##########
parquet-column/src/main/java/org/apache/parquet/internal/column/columnindex/IndexIterator.java:
##########
@@ -55,6 +55,105 @@ static PrimitiveIterator.OfInt rangeTranslate(int from, int to, IntUnaryOperator
return new IndexIterator(from, to + 1, i -> true, translator);
}
+ static PrimitiveIterator.OfInt intersection(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ int result = next;
+ next = fetchNext();
+ return result;
+ }
+
+ @Override
+ public boolean hasNext() {
+ return next != -1;
+ }
+
+ private int fetchNext() {
+ if (!lhs.hasNext() || !rhs.hasNext()) {
+ return -1;
+ }
+
+ // Since we know both iterators are in sorted order, we can iterate linearly through until
+ // we find the next value that belongs to both iterators, or terminate if none exist
+ int nextL = lhs.next();
+ int nextR = rhs.next();
+ while (nextL < nextR && lhs.hasNext()) {
+ nextL = lhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
+ while (nextR < nextL && rhs.hasNext()) {
+ nextR = rhs.next();
+ }
+ if (nextL == nextR) {
+ return nextL;
+ }
+
+ return -1;
+ }
+ };
+ }
+
+ static PrimitiveIterator.OfInt union(PrimitiveIterator.OfInt lhs, PrimitiveIterator.OfInt rhs) {
+ return new PrimitiveIterator.OfInt() {
+ private int peekL = -1;
+ private int peekR = -1;
+ private int next = fetchNext();
+
+ @Override
+ public int nextInt() {
+ int result = next;
Review Comment:
Missing exception, see above.
--
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: Add #contains FilterPredicate for Array columns [parquet-java]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #1328:
URL: https://github.com/apache/parquet-java/pull/1328
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "gszadovszky (via GitHub)" <gi...@apache.org>.
gszadovszky commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1580931878
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java:
##########
@@ -315,6 +321,66 @@ public <R> R accept(Visitor<R> visitor) {
}
}
+ public abstract static class ArrayContainsFilterPredicate<T extends Comparable<T>>
Review Comment:
It seems this class does not need to be public. Let's have it package private.
##########
parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java:
##########
@@ -315,6 +321,66 @@ public <R> R accept(Visitor<R> visitor) {
}
}
+ public abstract static class ArrayContainsFilterPredicate<T extends Comparable<T>>
+ implements FilterPredicate, Serializable {
+ private final Column<T> elementColumn;
+ private final T value;
+
+ protected ArrayContainsFilterPredicate(Column<T> elementColumn, T value) {
+ this.elementColumn = Objects.requireNonNull(elementColumn, "elementColumn cannot be null");
+ this.value = value;
+ }
+
+ public Column<T> getColumn() {
+ return elementColumn;
+ }
+
+ public T getValue() {
+ return value;
+ }
+
+ @Override
+ public String toString() {
+ String name = getClass().getSimpleName().toLowerCase(Locale.ENGLISH);
+ return name + "(" + elementColumn.getColumnPath().toDotString() + ", " + value + ")";
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (this == o) return true;
+ if (o == null || getClass() != o.getClass()) return false;
+ ArrayContainsFilterPredicate<?> that = (ArrayContainsFilterPredicate<?>) o;
+ return elementColumn.equals(that.elementColumn) && value.equals(that.value);
Review Comment:
It seems that `value` can be `null`. In this case it'll throw an NPE.
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/dictionarylevel/DictionaryFilter.java:
##########
@@ -487,6 +489,92 @@ public <T extends Comparable<T>> Boolean visit(NotIn<T> notIn) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Contains<T> contains) {
+ T value = contains.getValue();
+
+ // Dictionary only contains non-null values, so we can't use it to filter here
+ if (value == null) {
+ return BLOCK_MIGHT_MATCH;
+ }
+
+ Column<T> filterColumn = contains.getColumn();
+ ColumnChunkMetaData meta = getColumnChunk(filterColumn.getColumnPath());
+
+ if (meta == null) {
+ // the column isn't in this file so all values are null, but the value
+ // must be non-null because of the above check.
+ return BLOCK_CANNOT_MATCH;
+ }
+
+ // if the chunk has non-dictionary pages, don't bother decoding the
+ // dictionary because the row group can't be eliminated.
+ if (hasNonDictionaryPages(meta)) {
+ return BLOCK_MIGHT_MATCH;
+ }
+
+ try {
+ Set<T> dictSet = expandDictionary(meta);
+ if (dictSet != null && dictSet.contains(value)) {
+ return BLOCK_MIGHT_MATCH;
+ } else {
+ return BLOCK_CANNOT_MATCH;
+ }
+ } catch (IOException e) {
+ LOG.warn("Failed to process dictionary for filter evaluation.", e);
+ }
+ return BLOCK_MIGHT_MATCH; // cannot drop the row group based on this dictionary
+ }
+
+ @Override
+ public <T extends Comparable<T>> Boolean visit(DoesNotContain<T> doesNotContain) {
+ T value = doesNotContain.getValue();
+
+ Column<T> filterColumn = doesNotContain.getColumn();
+ ColumnChunkMetaData meta = getColumnChunk(filterColumn.getColumnPath());
+
+ if (value == null) {
+ if (meta == null) {
+ // @Todo ??
Review Comment:
Why the TODO? No metadata means the column is not in the file so every value is null. Therefore returning `BLACK_CANNOT_MATCH` is correct.
--
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: Add #contains FilterPredicate for Array columns [parquet-mr]
Posted by "clairemcginty (via GitHub)" <gi...@apache.org>.
clairemcginty commented on code in PR #1328:
URL: https://github.com/apache/parquet-mr/pull/1328#discussion_r1576808087
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java:
##########
@@ -119,6 +119,16 @@ public <T extends Comparable<T>> Boolean visit(Operators.GtEq<T> gtEq) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Operators.Contains<T> contains) {
+ return BLOCK_MIGHT_MATCH;
Review Comment:
I was testing this out with the BloomFilterVisitor and couldn't produce a test Parquet file with a populated bloom filter for a List field (I tried strings, ints, and longs as element types). I guess this is expected?
##########
parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java:
##########
@@ -119,6 +119,16 @@ public <T extends Comparable<T>> Boolean visit(Operators.GtEq<T> gtEq) {
return BLOCK_MIGHT_MATCH;
}
+ @Override
+ public <T extends Comparable<T>> Boolean visit(Operators.Contains<T> contains) {
+ return BLOCK_MIGHT_MATCH;
Review Comment:
I was testing this out with the BloomFilter Visitor and couldn't produce a test Parquet file with a populated bloom filter for a List field (I tried strings, ints, and longs as element types). I guess this is expected?
--
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