Support reading Parquet row group bloom filter

[huaxingao]

Co-Authored-By: Xi Chen [email protected]
Co-Authored-By: Hao Lin [email protected]
Co-Authored-By: Huaxin Gao [email protected]

This is the read path of parquet row group bloom filter. The original PR is here

[huaxingao]

I don't have a good way to test the read path, but I have tested these changes with the write path on my local.

[huaxingao]

cc @rdblue @RussellSpitzer @kbendick @chenjunjiedada @hililiwei

[rdblue]

Is it possible to include the unit test in this PR? I think all you'd need to do is configure the bloom filter settings using the Parquet settings in a Hadoop Configuration rather than through the Iceberg write settings.

[huaxingao]

I tried to config using ParquetOutputFormat.BLOOM_FILTER_ENABLED by replacing this line with .set(ParquetOutputFormat.BLOOM_FILTER_ENABLED + "#_id", "true") , but it doesn't work.

Seems Iceberg only honors the Iceberg's properties (which are set by Context at here), but it doesn't really take the properties set by Parquet.

[rdblue]

I think that properties will be passed through to the Hadoop Configuration automatically. Is that no longer true?

[huaxingao]

@rdblue Thanks for your quick reply!

Seems the properties need to be passed to InternalParquetRecordWriter through this encodingProps. We need to call the withXXX method explicitly e.g. withDictionaryPageSize to set the property to encodingPropsBuilder

So it seems to me that we have to call this withBloomFilterEnabled explicitly to set the bloom filter property toencodingPropsBuilder. Otherwise, Parquet's InternalParquetRecordWriter won't be able to take it.

[huaxingao]

Should I set the Parquet properties to encodingPropsBuilder? If the same properties is also set by iceberg properties, I will reset to overwrite.

[rdblue]

I see. Is it possible to get some tests working without the write-side changes? Maybe write a Parquet file directly and use name mapping? If not then let's try to make the minimal write-side changes to get the test in.

[huaxingao]

I made some minimal write-side changes to get the test in. Hope this is OK.

[rdblue]

Can we detect that an expression is unbound? Maybe identify named refs and return? Then we could just use one method for everything.

[huaxingao]

Can i just use instanceof Unbound to detect if an expression is unbound? Something like this:

  private static boolean isUnbound(Expression expr) {
    switch (expr.op()) {
      case TRUE:
        return false;
      case FALSE:
        return false;
      case NOT:
        Not not = (Not) expr;
        return isUnbound(not.child());
      case AND:
        And and = (And) expr;
        return isUnbound(and.left()) || isUnbound(and.right());
      case OR:
        Or or = (Or) expr;
        return isUnbound(or.left()) || isUnbound(or.right());
      default:
        return expr instanceof Unbound;
    }
  }

Then the method boundReferences can be

  public static Set<Integer> boundReferences(StructType struct, List<Expression> exprs, boolean caseSensitive) {
    if (exprs == null) {
      return ImmutableSet.of();
    }
    ReferenceVisitor visitor = new ReferenceVisitor();
    for (Expression expr : exprs) {
      if (isUnbound(expr)) {
        ExpressionVisitors.visit(bind(struct, expr, caseSensitive), visitor);
      } else {
        ExpressionVisitors.visit(expr, visitor);
      }
    }
    return visitor.references;
  }

[rdblue]

Not all expressions are bound or unbound. What about just trying to bind the expression and catching the exception that is thrown if it's already bound? Then you can just move on and return the references.

[huaxingao]

Sounds good to me!

[rdblue]

Style nit: the starting point of all argument lines should be aligned. That can be at 2 indents from the start of the method OR after the opening ( to start method arguments. There should not be two different indentation levels like you have here (both after ( and 2 indents from the method definition line).

[huaxingao]

Thanks for the reminder. I will pay attention to the style next time.

[rdblue]

We shouldn't rely on exception messages like this.

I think instead we should just add a utility to detect whether an expression is bound. Here's an implementation:

  /**
   * Returns whether an expression is bound.
   * <p>
   * An expression is bound if all of its predicates are bound.
   *
   * @param expr an {@link Expression}
   * @return true if the expression is bound
   * @throws IllegalArgumentException if the expression has both bound and unbound predicates.
   */
  public static boolean isBound(Expression expr) {
    Boolean isBound = ExpressionVisitors.visit(expr, new IsBoundVisitor());
    return isBound != null ? isBound : false; // assume unbound if undetermined
  }

  private static class IsBoundVisitor extends ExpressionVisitors.ExpressionVisitor<Boolean> {
    @Override
    public Boolean not(Boolean result) {
      return result;
    }

    @Override
    public Boolean and(Boolean leftResult, Boolean rightResult) {
      return combineResults(leftResult, rightResult);
    }

    @Override
    public Boolean or(Boolean leftResult, Boolean rightResult) {
      return combineResults(leftResult, rightResult);
    }

    @Override
    public <T> Boolean predicate(BoundPredicate<T> pred) {
      return true;
    }

    @Override
    public <T> Boolean predicate(UnboundPredicate<T> pred) {
      return false;
    }

    private Boolean combineResults(Boolean isLeftBound, Boolean isRightBound) {
      if (isLeftBound != null) {
        Preconditions.checkArgument(isRightBound == null || isLeftBound.equals(isRightBound),
            "Found partially bound expression");
        return isLeftBound;
      } else {
        return isRightBound;
      }
    }
  }

[huaxingao]

Sounds great! Thank you very much for the implementation!

[rdblue]

No problem! Thanks for all your work on this.

[rdblue]

I think this is okay for now to get this in with tests. Thanks, @huaxingao!

[rdblue]

Thanks, @huaxingao!

[huaxingao]

Thank you very much @rdblue! Also thank you @RussellSpitzer @kbendick @chenjunjiedada @hililiwei @stevenzwu

[kbendick]

Thank you for making support for reading Parquet bloom filters happen @huaxingao! Your work is highly appreciated. This will be very valuable for a number of use cases.