Parquet: Support In predicate pushdown for ParquetFilters

[jshmchenxi]

In predicate support was added to Parquet FilterApi via apache/parquet-java#923.

This PR adds In predicate support to Iceberg ParquetFilters, so that we can use Parquet native In predicate support for filters. This will be useful for page skipping feature as continuation work of #10228 and #10399.

As reference, support of native In predicate was added to Spark parquet reader in apache/spark#36696.

[jshmchenxi]

cc @huaxingao @rdblue

Any suggestion of how this should be tested would be appreciated. Currently I don't see any test for the class ParquetFilters.

[RussellSpitzer]

This looks generally good to me but I'm wondering if we have adequate test coverage for the feature? Do we have tests that are using the IN predicate already that cover this?

[RussellSpitzer]

Is this how this is handled in Spark? I may be in the minority here but I would prefer two different pred functions, one which uses Set and the other using C. Then only calling the one which is appropriate based on usage rather than having both arguments and using different args based on the predicate type.

[jshmchenxi]

Spark handles predicate pushdown in a more verbose way, basically:

predicate match {
    case OP1:
        parquetType match {
            case TYPE1: FilterApi.op1()
            case TYPE2: FilterApi.op1()
            ...
       }
    case OP2:
        parquetType match {
            case TYPE1: FilterApi.op2()
            case TYPE2: FilterApi.op2()
            ...
       }
    ...
}

While Iceberg handles this more compactly. It already handles unary predicate and literal predicate in the same pred function. I wouldn't mind adding a new pred function, but it seems we already use different args (no arg or C) based on predicate type in pred.

[RussellSpitzer]

cc @huaxingao @rdblue

Any suggestion of how this should be tested would be appreciated. Currently I don't see any test for the class ParquetFilters.

I think this is always used via Parquet.java

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Line 1417 in 05cd8b5

.withFilter(ParquetFilters.convert(fileSchema, filter, caseSensitive));

So I think almost all of our current coverage is downstream of actual implementations using Parquet.read.filter() which is a problem. So Theoretically and test with an IN clause in it for any engine should hit this but I feel like we should probably have some explicit unit tests of this?

[jshmchenxi]

This looks generally good to me but I'm wondering if we have adequate test coverage for the feature? Do we have tests that are using the IN predicate already that cover this?

It seems the existing tests using IN predicate are from row group filters:

• data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java
• parquet/src/test/java/org/apache/iceberg/parquet/TestBloomRowGroupFilter.java
• parquet/src/test/java/org/apache/iceberg/parquet/TestDictionaryRowGroupFilter.java

Iceberg currently lacks support for page skipping, thus the pushed Parquet filter actually does not help filtering records inside a row group IIUC. A working test case for predicate pushdown can be added along with page skipping feature, similar to https://github.com/apache/iceberg/pull/10228/files#diff-2fc3ebd599749d66bd75f9eb14e030aa97f2fc09e775d7ba372e73f46db25da8.

[jshmchenxi]

cc @huaxingao @rdblue
Any suggestion of how this should be tested would be appreciated. Currently I don't see any test for the class ParquetFilters.

I think this is always used via Parquet.java

iceberg/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java

Line 1417 in 05cd8b5

.withFilter(ParquetFilters.convert(fileSchema, filter, caseSensitive));

So I think almost all of our current coverage is downstream of actual implementations using Parquet.read.filter() which is a problem. So Theoretically and test with an IN clause in it for any engine should hit this but I feel like we should probably have some explicit unit tests of this?

Sounds good. I'll add explicit unit tests for ParquetFilters.convert.

[shangxinli]

if we add everything from litSet to convertedSet, should we initialize it with the size parameter as Sets.newHashSet(litSet); It would be more efficient.

[github-actions]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

[github-actions]

This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.