PARQUET-34: implement not() for Contains predicate by clairemcginty · Pull Request #2941 · apache/parquet-java

clairemcginty · 2024-07-04T16:19:07Z

This should be the final subtask for the Contains() predicate work. It adds support for logical inversion -- not(contains(...)).

Also, it fixes a small bug with the composition of mixed Contains predicates.

Make sure you have checked all steps below.

Issue

My PR addresses the following GitHub issues and references
them in the PR title. For example, "PARQUET-2410: Use row count instead of value count to get row count from OffsetIndex #1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-34 (should I make a new Github issue for this?)
- In case you are adding a dependency, check if the license complies with
  the ASF 3rd Party License Policy.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference GitHub issues in their subject lines. In addition, my commits follow the guidelines
from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including GitHub issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Style

My contribution adheres to the code style guidelines and Spotless passes.
- To apply the necessary changes, run mvn spotless:apply -Pvector-plugins

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain Javadoc that explain what it does

…umns

clairemcginty · 2024-07-04T16:21:16Z

parquet-column/src/main/java/org/apache/parquet/filter2/predicate/ContainsRewriter.java

-        throw new UnsupportedOperationException(
-            "Contains predicates cannot be composed with non-Contains predicates");
-      }
+    // If two Contains predicates refer to the same column, optimize by combining into a single predicate


This fixes a bug with the way Contains predicates were composed -- previously it would throw an error if you tried to do something like and(contains(binaryColumn("foo", ...), contains(binaryColumn("bar"), ...)) because the columns didn't match. however, that is still a valid predicate -- it's just that they should not be attempted to be combined into a single Contains predicate.

clairemcginty · 2024-07-04T16:23:31Z

...rc/main/java/org/apache/parquet/filter2/recordlevel/IncrementallyUpdatedFilterPredicate.java

+  /**
+   * A ValueInspector implementation that keeps state for one or more delegate inspectors.
+   */
+  abstract static class DelegatingValueInspector extends ValueInspector {


I created this because I thought the generated IncrementallyUpdatedFilterPredicateBuilder was getting complex in terms of LOC -- all the Contains ValueInspectors were overriding public void update(int|float|binary|double|... value) as well as reset(), and it was getting hard to reason about. However.... this is probably a bit less efficient, since the delegate ValueInspectors are accessed via iterator loops rather than directly invoking left/right. Let me know what you think, I'm happy to change it back to the way it was

IncrementallyUpdatedFilterPredicateGenerator.java is pretty difficult to read and review. Since this is only applied to ContainsPredicate and does not affect others, I'm fine to make this change.

clairemcginty · 2024-07-04T18:49:38Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

    @Override
    public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
-      return contains.filter(this, IndexIterator::intersection, IndexIterator::union);
+      return contains.filter(


My initial idea was to implement IndexIterator::subtract for not(contains()) -- for example, not(contains(eq("bar"))) would return something like IndexIterator.all(getPageCount()).subtract(visit(eq("bar")). But we can't rely on the individual predicate implementations to return exact row ranges -- for example, NotIn always returns IndexIterator.all. So I don't think it's possible to safely subtract any row ranges here.

Yes, we cannot make such subtraction unless it is a single-value page which has only bar.

wgtmac

Thanks for the change! I just took an initial pass and has some questions overall.

parquet-hadoop/src/main/java/org/apache/parquet/filter2/statisticslevel/StatisticsFilter.java

wgtmac · 2024-07-08T02:19:35Z

...rc/main/java/org/apache/parquet/filter2/recordlevel/IncrementallyUpdatedFilterPredicate.java

+  /**
+   * A ValueInspector implementation that keeps state for one or more delegate inspectors.
+   */
+  abstract static class DelegatingValueInspector extends ValueInspector {


IncrementallyUpdatedFilterPredicateGenerator.java is pretty difficult to read and review. Since this is only applied to ContainsPredicate and does not affect others, I'm fine to make this change.

wgtmac · 2024-07-08T02:26:51Z

...-column/src/main/java/org/apache/parquet/internal/column/columnindex/ColumnIndexBuilder.java

    @Override
    public <T extends Comparable<T>> PrimitiveIterator.OfInt visit(Contains<T> contains) {
-      return contains.filter(this, IndexIterator::intersection, IndexIterator::union);
+      return contains.filter(


Yes, we cannot make such subtraction unless it is a single-value page which has only bar.

wgtmac · 2024-07-08T02:28:27Z

parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java

+        Visitor<R> visitor,
+        BiFunction<R, R, R> andBehavior,
+        BiFunction<R, R, R> orBehavior,
+        Function<R, R> notBehavior);


IIUC, all kinds of filters return all rows if notBehavior is found, right?

Yes, for example NotIn 👍

Co-authored-by: Gang Wu <ustcwg@gmail.com>

wgtmac

+1

wgtmac · 2024-07-25T05:18:41Z

@gszadovszky Do you want to take a look?

wgtmac · 2024-07-30T02:05:18Z

Merged. Thanks @clairemcginty and @gszadovszky!

clairemcginty added 2 commits July 3, 2024 11:30

PARQUET-34: implement not() for Contains predicate

6d22107

PARQUET-34: Allow composition of COntains predicates on different col…

c26624f

…umns

clairemcginty commented Jul 4, 2024

View reviewed changes

clairemcginty added 2 commits July 4, 2024 13:06

PARQUET-34: Test toString for not(contains())

20a4e29

PARQUET-34: Make DoesNotContain private

7b8a4c8

wgtmac reviewed Jul 8, 2024

View reviewed changes

PARQUET-34: Rename lambda variable

1ca2207

Co-authored-by: Gang Wu <ustcwg@gmail.com>

clairemcginty force-pushed the parquet-34-not-contains branch from 3974c69 to 1ca2207 Compare July 10, 2024 13:22

wgtmac approved these changes Jul 17, 2024

View reviewed changes

gszadovszky approved these changes Jul 25, 2024

View reviewed changes

wgtmac merged commit ea3d273 into apache:master Jul 30, 2024

wgtmac added this to the 1.15.0 milestone Sep 30, 2024

Conversation

clairemcginty commented Jul 4, 2024

Issue

Tests

Commits

Style

Documentation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wgtmac left a comment

Choose a reason for hiding this comment

Uh oh!

wgtmac commented Jul 25, 2024

Uh oh!

wgtmac commented Jul 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants