IllegarlArgumentException when filtering on BinaryType column.

[cccs-br]

We've encountered a scenario where iceberg throws an IllegalArgumentException when filtering a dataframe against a column that is a BinaryType.

Here is the stacktrace

ava.lang.IllegalArgumentException: Cannot convert bytes to SQL literal: java.nio.HeapByteBuffer[pos=0 lim=16 cap=16]

	at org.apache.iceberg.spark.Spark3Util$DescribeExpressionVisitor.sqlString(Spark3Util.java:654)
	at org.apache.iceberg.spark.Spark3Util$DescribeExpressionVisitor.predicate(Spark3Util.java:628)
	at org.apache.iceberg.spark.Spark3Util$DescribeExpressionVisitor.predicate(Spark3Util.java:576)
	at org.apache.iceberg.expressions.ExpressionVisitors.visit(ExpressionVisitors.java:308)

here

In order to reproduce the issue, you can simply clone this repo https://github.com/cccs-br/spark-iceberg-issue

git@github.com:cccs-br/spark-iceberg-issue.git

The test case reproducing the exception is here:

https://github.com/cccs-br/spark-iceberg-issue/blob/f7811250608df8618fe264cbf340b5d40effba0d/src/test/java/IcebergTests.java#L59

You will see in this class three more test cases that show very similar filtering but are successful.

Out of the 4 test cases in this class, only one fails, where you would expect all of them to succeed.

The anomaly here is that the filtering works when using a literal expression, but fails when using:

Java

spark.sql("select * from my_table").where(col("binary_column_name").$greater(my_byte_array))

Scala

spark.sql("select * from my_table").where(col("binary_column_name") > my_byte_array)

Two of the tests are basically reproducing the two cases. The difference being that the DataFrame is created by reading the parquet files generated by iceberg directly. This is just to show that both filtering methods work in those instances.

As far as I can tell, this should be working. But I could be wrong. If so, can you advise?

We're using Spark-3.1.2 with

        <dependency>
            <groupId>org.apache.iceberg</groupId>
            <artifactId>iceberg-spark3-runtime</artifactId>
            <version>0.11.1</version>
            <scope>tests</scope>
        </dependency>

[kbendick]

Have you tried using the lit function to see if that handles it?

Also, correct me if I'm wrong, but there should be a using iceberg here in the create table DDL? https://github.com/cccs-br/spark-iceberg-issue/blob/main/src/test/java/IcebergTestUtil.java#L46

[kbendick]

Looking at the linked code, it does appear like ByteBuffer is intentionally excluded. I'm guessing because there's no SQL syntax (at least in Spark) for representing byte buffers / byte arrays?

[cccs-br]

Thanks for you suggestions. But I added 'USING iceberg' to the create table sql. And also added a test case that wraps the byte array with 'lit'. As suspected, the issue remains.

The question is:

why does this one work:

https://github.com/cccs-br/spark-iceberg-issue/blob/f7811250608df8618fe264cbf340b5d40effba0d/src/test/java/IcebergTests.java#L40

..and this one fails:

https://github.com/cccs-br/spark-iceberg-issue/blob/f7811250608df8618fe264cbf340b5d40effba0d/src/test/java/IcebergTests.java#L50

So, it does appear like spark handles it, but going through iceberg, it fails. Spark3Util.java:654 is where the exception is explicitly thrown.

[RussellSpitzer]

Probably has to do with our supporters metric evaluators, I bet if you debugged the working one spark translates that into some other type. We had a similar issue with new date and local time objects which spark doesn’t generate itself but you can pass through as literals

…

Sent from my iPhone

On Aug 6, 2021, at 9:01 AM, cccs-br ***@***.***> wrote:  Thanks for you suggestions. But I added 'USING iceberg' to the create table sql. And also added a test case that wraps the byte array with 'lit'. As suspected, the issue remains. The question is: why does this one work: https://github.com/cccs-br/spark-iceberg-issue/blob/f7811250608df8618fe264cbf340b5d40effba0d/src/test/java/IcebergTests.java#L40 ..and this one fails: https://github.com/cccs-br/spark-iceberg-issue/blob/f7811250608df8618fe264cbf340b5d40effba0d/src/test/java/IcebergTests.java#L50 — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[cccs-br]

Thanks @RussellSpitzer . Is that something that you guys fixed in iceberg directly? - or you had to adjust your queries to get around it.

[RussellSpitzer]

This is something that needs to be fixed in Iceberg, we haven't run into this yet but @manishmalhotrawork and I were talking about possible fixes over the weekend.

I'm pretty sure the issue here is in predicate translation, we have a function sparkSql() which doesn't handle ByteBuffers even though those are valid predicates in evaluators. See

iceberg/spark3/src/main/java/org/apache/iceberg/spark/Spark3Util.java

Lines 718 to 721 in 40e626a

	if (lit.value() instanceof String) {
	return "'" + lit.value() + "'";
	} else if (lit.value() instanceof ByteBuffer) {
	throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);

I think we just need to convert incoming byteBuffers to byteArrays here

iceberg/spark3/src/main/java/org/apache/iceberg/spark/SparkFilters.java

Lines 198 to 209 in 40e626a

	private static Object convertLiteral(Object value) {
	if (value instanceof Timestamp) {
	return DateTimeUtils.fromJavaTimestamp((Timestamp) value);
	} else if (value instanceof Date) {
	return DateTimeUtils.fromJavaDate((Date) value);
	} else if (value instanceof Instant) {
	return DateTimeUtils.instantToMicros((Instant) value);
	} else if (value instanceof LocalDate) {
	return DateTimeUtils.localDateToDays((LocalDate) value);
	}
	return value;
	}

So that the rest of our SparkUtil code will work with byte buffer literals generated by Spark

[cccs-br]

Ok, great. Will you reference this issue in PR when it comes in? I'd just like to know when this gets merged and eventually released so we can upgrade our iceberg lib as soon as it becomes available.

Thanks again.

[cccs-br]

Hi everyone, ...was just wondering if there are any updates on this? As always, thank you.

[manishmalhotrawork]

hi @cccs-br
Very sorry for the delay!
Got sidelined with other stuff.

I was thinking to take this up, also spend some time initially, and reach to a place where found that:

looks like org.apache.iceberg.expressions.Literals.from is the place where byte[] or ByteBuffer converted to the corresponding literals

static <T> Literal<T> from(T value) {
    Preconditions.checkNotNull(value, "Cannot create expression literal from null");
    Preconditions.checkArgument(!NaNUtil.isNaN(value), "Cannot create expression literal from NaN");

    if (value instanceof Boolean) {
      return (Literal<T>) new Literals.BooleanLiteral((Boolean) value);
    } else if (value instanceof Integer) {
      return (Literal<T>) new Literals.IntegerLiteral((Integer) value);
    } else if (value instanceof Long) {
      return (Literal<T>) new Literals.LongLiteral((Long) value);
    } else if (value instanceof Float) {
      return (Literal<T>) new Literals.FloatLiteral((Float) value);
    } else if (value instanceof Double) {
      return (Literal<T>) new Literals.DoubleLiteral((Double) value);
    } else if (value instanceof CharSequence) {
      return (Literal<T>) new Literals.StringLiteral((CharSequence) value);
    } else if (value instanceof UUID) {
      return (Literal<T>) new Literals.UUIDLiteral((UUID) value);
    } else if (value instanceof byte[]) {
      return (Literal<T>) new Literals.FixedLiteral(ByteBuffer.wrap((byte[]) value));
    } else if (value instanceof ByteBuffer) {
      return (Literal<T>) new Literals.BinaryLiteral((ByteBuffer) value);
    } else if (value instanceof BigDecimal) {
      return (Literal<T>) new Literals.DecimalLiteral((BigDecimal) value);
    }

Also discussed with @RussellSpitzer and next step was to check the query plan in spark sql case. That can give more insight as how it's being handled in case of spark sql.

thanks @RussellSpitzer for the help!

[cccs-br]

That is awesome @manishmalhotrawork . Thanks again for looking into this.

[cccs-br]

Closed by accident. Reopening. Sry about that.