Spark: Generate Spark's SQL literal format for binary and update the toString representation of binary/fixed literals. #3728

xiaotianzhang01 · 2021-12-13T09:25:35Z

adds the ability to assert binary arrays for "assertEquals" and adds iceberg hexadecimal text parsing

solve cannot convert bytes to SQL literal

spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java

kbendick · 2021-12-15T04:06:46Z

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java

          } else if (value instanceof scala.collection.Map) {
            return row.getJavaMap(pos);
+          } else if (value.getClass().isArray() && value.getClass().getComponentType().isPrimitive()) {
+            return IntStream.range(0, Array.getLength(value)).mapToObj(i -> Array.get(value, i)).toArray();


Question: This change to the test class seems somewhat complicated relative to the other cases. Is there any way to check for value instanceof scala.collection.Array (assuming that Scala types are returned) like in the map case and in the basic list case?

I guess my question is, why do we need this block when we have the check for scala.collection.Seq already? Can you help me understand?

And possibly, since it is a bit more complicated, a comment addressing what case this is expected to handle in the code might be called for.

yeap, i’ll simplify the code and add instructions

What does this convert the byte array to? Why does it need to change? I thought byte[] was fine to return.

The assertEquals method cannot verify whether binary arrays are equal. If we do not convert binary arrays to Object arrays, we may need to create a separate method to compare binary arrays

This representation should not change for tests. I think that the original type is correct. If you need to fix tests that are failing because of array comparison, then you should update the assertions instead of runtime code.

rdblue · 2021-12-17T17:44:37Z

api/src/main/java/org/apache/iceberg/expressions/Literals.java

+          return "0x" + BaseEncoding.base16().encode((toByteBuffer().array()));
+        default:
+          return String.valueOf(value);
+      }


This method should not change. It is the base implementation. Instead, you should override the implementation in BinaryLiteral and FixedLiteral.

ok, thanks for your reply

rdblue · 2021-12-17T17:45:21Z

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java

-    sql("CREATE TABLE %s (id bigint, data string, float float) USING iceberg", tableName);
-    sql("INSERT INTO %s VALUES (1, 'a', 1.0), (2, 'b', 2.0), (3, 'c', float('NaN'))", tableName);
+    sql("CREATE TABLE %s (id bigint, data string, float float, binary binary) USING iceberg", tableName);
+    sql("INSERT INTO %s VALUES (1, 'a', 1.0, X''), (2, 'b', 2.0, X'1111'), (3, 'c', float('NaN'), X'11')", tableName);


Please don't edit existing test cases. Instead, add new test cases/methods for what you are changing.

ok, thanks for your comment

rdblue · 2021-12-17T17:47:23Z

@xiaotianzhang01 can you fix the description and summary for this? This is not adding any parsing to Iceberg, so the title is misleading. Instead, it producing Spark's SQL literal format for binary and updating the toString representation of binary/fixed literals.

xiaotianzhang01 · 2021-12-23T09:08:31Z

@rdblue @kbendick Please take a look at it again.

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java

xiaotianzhang01 · 2021-12-30T02:57:01Z

@rdblue Thank you very much for your review! I have addressed your comments

rdblue · 2022-01-04T17:57:26Z

api/src/main/java/org/apache/iceberg/expressions/Literals.java

+
+    @Override
+    public String toString() {
+      return "X'" + BaseEncoding.base16().encode((toByteBuffer().array())) + "'";


It is not safe to use ByteBuffer.array like this. Can you please use ByteBuffers.toByteArray?

okey, update

…oString representation of binary/fixed literals.

rdblue · 2022-01-09T19:32:34Z

spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java

      } else if (lit.value() instanceof ByteBuffer) {
-        throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
+        // produce a literal value that Spark can parse
+        return lit.toString();


It is fine to update BinaryLiteral.toString and FixedLiteral.toString, but this should not assume that the toString representation of a literal can be parsed by Spark. Instead, this should produce the correct form here, just like it does for String literals.

If we implement BinaryLiteral.toString and FixedLiteral.toString, then we don't need to do it separately for ByteBuffer

No, we should not rely on toString using a representation that works for Spark. Instead, this should convert the literal for Spark here. We may change the toString representation in Literal and we don't want this to depend on that method. It's fine for numbers, but there are different representations for binary that are equally valid. This is where we customize the one that Spark uses.

Okay, got it

rdblue · 2022-01-10T18:39:19Z

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java

+        if (expectedValue instanceof byte[]) {
+          Assert.assertArrayEquals(newContext, (byte[]) expectedValue, (byte[]) actualValue);
+        } else {
+          assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);


Why does the assertEquals with Object[] not work for byte[]?

The SQL query returns byte[] instead of Byte[]. The encapsulated type Byte[] can be converted to Object[], but the conversion of the basic type byte[] will report an error, [B cannot be cast to [Ljava.lang.Object;

rdblue · 2022-01-12T16:55:52Z

Thanks, @xiaotianzhang01! Nice work.

izchen · 2022-01-13T06:08:56Z

Nice work.

github-actions bot added API spark labels Dec 13, 2021

kbendick reviewed Dec 13, 2021

View reviewed changes

spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java Outdated Show resolved Hide resolved

kbendick reviewed Dec 15, 2021

View reviewed changes

rdblue reviewed Dec 17, 2021

View reviewed changes

xiaotianzhang01 changed the title ~~Spark: adds the ability to parse iceberg hexadecimal text~~ Spark: Generate Spark's SQL literal format for binary and update the toString representation of binary/fixed literals. Dec 22, 2021

xiaotianzhang01 force-pushed the hexadecimal branch from d7f5c4d to a9f01e6 Compare December 23, 2021 08:54

xiaotianzhang01 force-pushed the hexadecimal branch 2 times, most recently from fad6688 to e837b37 Compare December 23, 2021 13:29

xiaotianzhang01 requested review from kbendick and rdblue December 28, 2021 02:40

rdblue reviewed Dec 29, 2021

View reviewed changes

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java Outdated Show resolved Hide resolved

rdblue reviewed Dec 29, 2021

View reviewed changes

spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java Outdated Show resolved Hide resolved

rdblue reviewed Jan 4, 2022

View reviewed changes

Spark: produce Spark's SQL literal format for binary and update the t…

9ed6de3

…oString representation of binary/fixed literals.

xiaotianzhang01 force-pushed the hexadecimal branch from 49a8716 to 9ed6de3 Compare January 6, 2022 13:00

xiaotianzhang01 requested a review from rdblue January 6, 2022 13:21

rdblue reviewed Jan 9, 2022

View reviewed changes

xiaotianzhang01 requested a review from rdblue January 10, 2022 03:11

fix comments, update the assertions

b2b8273

xiaotianzhang01 force-pushed the hexadecimal branch from 3d284bc to b2b8273 Compare January 10, 2022 03:19

rdblue reviewed Jan 10, 2022

View reviewed changes

fix comments

cb94257

xiaotianzhang01 requested a review from rdblue January 11, 2022 02:35

rdblue approved these changes Jan 11, 2022

View reviewed changes

rdblue merged commit 68ed418 into apache:master Jan 12, 2022

rdblue mentioned this pull request Jan 12, 2022

Spark: Fix IllegarlArgumentException when filtering on BinaryType column #3460

Closed

xloya mentioned this pull request Sep 22, 2022

Spark3.1: Backport #3728 (Binary/Fixed literals) / #3983 (Zorder strategy) to Spark3.1 #5822

Closed

xloya pushed a commit to xloya/iceberg that referenced this pull request Sep 24, 2022

backport patch apache#3728

767d5ac

xloya mentioned this pull request Sep 24, 2022

Spark 3.1: Backport #3983 (Zorder strategy) to Spark 3.1 #5848

Closed

Spark: Generate Spark's SQL literal format for binary and update the toString representation of binary/fixed literals. #3728

Spark: Generate Spark's SQL literal format for binary and update the toString representation of binary/fixed literals. #3728

Uh oh!

Conversation

xiaotianzhang01 commented Dec 13, 2021

Uh oh!

Uh oh!

kbendick Dec 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

xiaotianzhang01 Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rdblue commented Dec 17, 2021

Uh oh!

xiaotianzhang01 commented Dec 23, 2021

Uh oh!

Uh oh!

Uh oh!

xiaotianzhang01 commented Dec 30, 2021

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!

rdblue commented Jan 12, 2022

Uh oh!

izchen commented Jan 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kbendick Dec 15, 2021 •

edited

Loading

xiaotianzhang01 Jan 6, 2022 •

edited

Loading