[PARQUET-1968] FilterApi support In predicate #923

huaxingao · 2021-08-14T15:19:57Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Parquet Jira issues and references them in the PR title. For example, "PARQUET-1234: My Parquet PR"
- https://issues.apache.org/jira/browse/PARQUET-1968
- 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 Jira 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 Jira 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"

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

huaxingao · 2021-08-16T16:16:46Z

@gszadovszky @shangxinli @rdblue Could you please take a look at this PR when you have time? Thanks a lot!

huaxingao · 2021-08-17T06:07:13Z

also cc @chenjunjiedada

gszadovszky

I have some comments in the code. All of them are more to open discussions so I neither approve nor disapprove for now.
Otherwise the code seems good to me. Thanks a lot for working on it!

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

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

pom.xml

shangxinli · 2021-08-30T15:43:49Z

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

    }
  }

+  // base class for In and NotIn


Have a better comment since it is public method

Fixed. Thanks!

shangxinli · 2021-08-30T15:54:02Z

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

+      this.values = Objects.requireNonNull(values, "values cannot be null");
+      checkArgument(!values.isEmpty(), "values in SetColumnFilterPredicate shouldn't be empty!");
+
+      String name = getClass().getSimpleName().toLowerCase(Locale.ENGLISH);


I see you have a 'toString' to cache but do we see generally this is reused multiple times? If no, proactively converting to string will be a waste.

shangxinli · 2021-08-30T15:55:35Z

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

+        iter++;
+      }
+      String valueStr = values.size() <= 100 ? str.substring(0, str.length() - 2) : str + "...";
+      this.toString = name + "(" + column.getColumnPath().toDotString() + ", " + valueStr + ")";


Would it be possible to merge lines 272 and 273 into the above code of that building? the string? String operations sometimes consume a lot of memory like this.

Is it just enough to replace str + "..." to str.append("...").toString?

str.substring(0, str.length() - 2) is still StringBuilder operation. Seems fine?

Maybe we can replace line 273 with StringBuilder operation too?

Fixed. Thanks!

shangxinli · 2021-08-30T15:57:22Z

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

+    @Override
+    public boolean equals(Object o) {
+      if (this == o) return true;
+      if (o == null || getClass() != o.getClass()) return false;


I guess you can just 'return this.getClass() == o.getClass()'

Yes, but just trying to follow the style at https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/filter2/predicate/Operators.java#L150

shangxinli · 2021-08-30T16:00:31Z

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

+      if (this == o) return true;
+      if (o == null || getClass() != o.getClass()) return false;
+      SetColumnFilterPredicate<?> that = (SetColumnFilterPredicate<?>) o;
+      return column.equals(that.column) && values.equals(that.values) && Objects.equals(toString, that.toString);


Is toString comparison still needed here? It seems toString have (values and class). You can just compare class here.

Removed toString comparison

…ions by throwing Exception

huaxingao · 2021-08-31T02:35:49Z

@gszadovszky @shangxinli @dbtsai Thank you all very much for reviewing! I have changed the code to generate the visit methods for in/notIn and also added the default by throwing Exception. Will address the rest of the comments tomorrow or the day after tomorrow.

gszadovszky

I have some more concrete comments in the code. Some more work is needed but I think it is going to the good direction. Thanks a lot for your efforts.

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

parquet-hadoop/src/main/java/org/apache/parquet/filter2/bloomfilterlevel/BloomFilterImpl.java

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

...umn/src/test/java/org/apache/parquet/internal/filter2/columnindex/TestColumnIndexFilter.java

...r/src/main/java/org/apache/parquet/filter2/IncrementallyUpdatedFilterPredicateGenerator.java

gszadovszky

Added a couple of more comments/requests.
Sorry if I am a bit strict here but filtering is not an easy topic and can have serious issues (lost of data).

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

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

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

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

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

...r/src/main/java/org/apache/parquet/filter2/IncrementallyUpdatedFilterPredicateGenerator.java

parquet-hadoop/src/test/java/org/apache/parquet/filter2/recordlevel/TestRecordLevelFilters.java

gszadovszky

I have a couple of comments for the new MinMax class but otherwise everything seems great!

parquet-column/src/main/java/org/apache/parquet/column/MinMax.java

gszadovszky · 2021-09-27T08:37:40Z

parquet-column/src/main/java/org/apache/parquet/column/MinMax.java

+      T element = iterator.next();
+      if (max == null) {
+        max = element;
+      } else if (max != null && element != null) {


You are already in the else path so do not need to check for max != null.

gszadovszky

I have some minor issues only. Thanks a lot for your efforts to implement this! I think it is a great improvement for the query engines.

parquet-column/src/main/java/org/apache/parquet/column/MinMax.java

huaxingao · 2021-09-30T14:22:01Z

@gszadovszky @shangxinli @viirya @dbtsai Thank you so much for all your help!!

gszadovszky · 2021-09-30T14:23:59Z

Thank you for your contribution, @huaxingao! Great work!

huaxingao added 4 commits August 13, 2021 19:13

[PARQUET-1968] FilterApi support In predicate

bc2d29e

fix white space

493b34c

fix white space

4b2d5a4

fix white space

a99ea78

gszadovszky reviewed Aug 25, 2021

View reviewed changes

shangxinli reviewed Aug 30, 2021

View reviewed changes

generate visit methods for in/notInt and also add default implementat…

e428745

…ions by throwing Exception

huaxingao added 2 commits August 30, 2021 19:46

remove exclude FilterPredicate from pom.xml

3ba34c5

address comments

507ed47

gszadovszky requested changes Sep 14, 2021

View reviewed changes

huaxingao added 5 commits September 14, 2021 22:28

address comments

02af9c4

/* -> /** for java doc

562888d

address comments

87d0791

fix white space

803d51d

fix white space

ac3ea9e

gszadovszky requested changes Sep 23, 2021

View reviewed changes

huaxingao added 2 commits September 26, 2021 16:12

address comments

2e77e28

fix white space

b0976aa

gszadovszky requested changes Sep 27, 2021

View reviewed changes

address comments

3571d43

gszadovszky requested changes Sep 28, 2021

View reviewed changes

address comments

65f2a35

gszadovszky approved these changes Sep 28, 2021

View reviewed changes

gszadovszky merged commit 98e3e1a into apache:master Sep 30, 2021

huaxingao deleted the in_filter branch September 30, 2021 14:22

jshmchenxi mentioned this pull request Sep 10, 2025

Parquet: Support In predicate pushdown for ParquetFilters apache/iceberg#14041

Closed

[PARQUET-1968] FilterApi support In predicate #923

[PARQUET-1968] FilterApi support In predicate #923

Uh oh!

Conversation

huaxingao commented Aug 14, 2021

Jira

Tests

Commits

Documentation

Uh oh!

huaxingao commented Aug 16, 2021

Uh oh!

huaxingao commented Aug 17, 2021

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

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!

huaxingao commented Aug 31, 2021

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gszadovszky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!