You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2021/11/03 17:35:50 UTC
[GitHub] [iceberg] izchen opened a new pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
izchen opened a new pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460
The test case reproducing the exception：
```sql
CREATE TABLE filter_binary_test (cbinary BINARY) USING ICEBERG;
SELECT cbinary FROM filter_binary_test WHERE cbinary > X'110F';
```
```
java.lang.IllegalArgumentException: Cannot convert bytes to SQL literal: java.nio.HeapByteBuffer[pos=0 lim=2 cap=2]
```
Like this SQL, BinaryLiteral(ByteBuffer) is not an illegal value.
in addition, the BinaryLiteral string format in this PR is the same as spark.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742232207
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-1011256177
This is fixed by #3728, which I have been reviewing lately and just committed. I didn't realize this was a separate PR until yesterday, so sorry that I hadn't come back to this one yet (I actually thought I was reviewing the same one!). In any case, the fix is nearly identical so this should be fixed by that other issue. Thanks, @izchen!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750612878
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
`sqlString` is implemented and used in the DescribeExpressionVisitor class.
I think this class is just for generating description information, and the description will be displayed in the log and Spark UI for debugging. So in my opinion, it is an acceptable way to use Literal.toString directly, which ensures that the description is the same as the style of other logs.
In fact, except for the difference between StringLiteral's single and double quotes, the current sqlString and Literal.toString are the same.
But if you think that keeping sqlString is a better way, I can also modify the code.
In addition, there are some differences between the description form of spark here, for example, the StringLiteral of spark does not have quotation marks. If you think that this should be the same as the behavior of spark, I can also modify the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742282785
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
This should just be for debugging. If we are converting a literal to a string and back, then that's a problem.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
@kbendick is right. This should not remove the `sqlString` function. That converts a literal into a form that is usable by Spark. Instead, you should update that function to produce Spark's binary literal format.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -157,7 +158,11 @@ private void assertEquals(String context, Object[] expectedRow, Object[] actualR
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
- assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ if (expectedValue.getClass().getComponentType().isPrimitive()) {
+ new ExactComparisonCriteria().arrayEquals(newContext, expectedValue, actualValue);
+ } else {
+ assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ }
Review comment:
Why was this needed?
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
Rather than repurposing old test cases, can you create new ones? We want to avoid mixing tests together in confusing ways that look like other failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742282785
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
This should just be for debugging. If we are converting a literal to a string and back, then that's a problem.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750613371
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -599,6 +600,13 @@ public String toString() {
Object writeReplace() throws ObjectStreamException {
return new SerializationProxies.FixedLiteralProxy(value());
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
Thanks for your review, I have modified the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-970628383
> Eventually, we'll need to update more than just Spark v3.2. Additionally, it would be nice if Flink and even MR could be handled in this PR.
### Flink
Executing this SQL test case in flink will report an error:
```
[ERROR] Could not execute SQL statement. Reason:
org.apache.flink.table.api.ValidationException: Data type 'BINARY(2) NOT NULL' with conversion class '[B' does not support a value literal of class 'org.apache.calcite.avatica.util.ByteString'.
```
It seems that this is an internal error of flink, which has nothing to do with iceberg. I can add the following code to the flink code to solve this problem (I will try to submit a PR to the flink community in the future):
```scala
case BINARY =>
// convert to byte[]
literal.getValueAs(classOf[Array[Byte]])
```
https://github.com/apache/flink/blob/44378fa5fde6c17e1712a62b834cb6251605f416/flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/plan/utils/RexNodeExtractor.scala#L400-L466
After using this code to fix flink, the SQL test case runs normally, returns the correct result, and the SQL description string in the flink UI is the correct X'110F'. I think this problem does not exist in Flink-runtime.
### Hive
According to the hive documentation, there is no binary type literal in hive. I think this problem does not exist in Hive-runtime.
### Trino
For Trino, I have not actually used Trino. Judging from the Trino code, Trino may not have this MR problem, but I do not have a Trino test environment. And we cannot modify the Trino-runtime code in this MR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750612304
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
This should just be for debugging.
In the base class, the toString method is `String.valueOf(value)` (In StringLiteral, it is `"\"" + value() + "\""`).
For other types of literals, the return value of toString is clear. But for ByteBuffer, the return value is `java.nio.HeapByteBuffer[pos=0 lim=16 cap=16]`.
So I implemented the literal toString method of ByteBuffer type, for logging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742284160
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
Rather than repurposing old test cases, can you create new ones? We want to avoid mixing tests together in confusing ways that look like other failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742283395
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
@kbendick is right. This should not remove the `sqlString` function. That converts a literal into a form that is usable by Spark. Instead, you should update that function to produce Spark's binary literal format.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750613710
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -157,7 +158,11 @@ private void assertEquals(String context, Object[] expectedRow, Object[] actualR
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
- assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ if (expectedValue.getClass().getComponentType().isPrimitive()) {
+ new ExactComparisonCriteria().arrayEquals(newContext, expectedValue, actualValue);
+ } else {
+ assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ }
Review comment:
Thanks for your review, I have modified the code
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
Thanks for your review, I have modified the code
##########
File path: api/src/main/java/org/apache/iceberg/util/ByteBuffers.java
##########
@@ -58,6 +59,11 @@ public static ByteBuffer copy(ByteBuffer buffer) {
return ByteBuffer.wrap(copyArray);
}
+ public static String encodeHexString(ByteBuffer buffer) {
+ byte[] bytes = toByteArray(buffer.duplicate());
Review comment:
Thanks for your review, I have modified the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-1006322589
Do you think this PR is necessary? if not, I will close this PR. Ping @rdblue @kbendick :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-959766469
Related issue: https://github.com/apache/iceberg/issues/2934
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742282785
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
This should just be for debugging. If we are converting a literal to a string and back, then that's a problem.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
@kbendick is right. This should not remove the `sqlString` function. That converts a literal into a form that is usable by Spark. Instead, you should update that function to produce Spark's binary literal format.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -157,7 +158,11 @@ private void assertEquals(String context, Object[] expectedRow, Object[] actualR
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
- assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ if (expectedValue.getClass().getComponentType().isPrimitive()) {
+ new ExactComparisonCriteria().arrayEquals(newContext, expectedValue, actualValue);
+ } else {
+ assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ }
Review comment:
Why was this needed?
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
Rather than repurposing old test cases, can you create new ones? We want to avoid mixing tests together in confusing ways that look like other failures.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
This should just be for debugging. If we are converting a literal to a string and back, then that's a problem.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
@kbendick is right. This should not remove the `sqlString` function. That converts a literal into a form that is usable by Spark. Instead, you should update that function to produce Spark's binary literal format.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -157,7 +158,11 @@ private void assertEquals(String context, Object[] expectedRow, Object[] actualR
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
- assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ if (expectedValue.getClass().getComponentType().isPrimitive()) {
+ new ExactComparisonCriteria().arrayEquals(newContext, expectedValue, actualValue);
+ } else {
+ assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ }
Review comment:
Why was this needed?
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
Rather than repurposing old test cases, can you create new ones? We want to avoid mixing tests together in confusing ways that look like other failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750613227
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -39,6 +39,7 @@
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.internal.ExactComparisonCriteria;
Review comment:
Thanks for your review, I have modified the code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750612878
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
`sqlString` is implemented and used in the DescribeExpressionVisitor class.
I think this class is just for generating description information, and the description will be displayed in the log and Spark UI for debugging. So in my opinion, it is an acceptable way to use `Literal.toString` directly, which ensures that the description is the same as the style of other logs.
In fact, except for the difference between StringLiteral's single and double quotes, the current `sqlString` and `Literal.toString` are the same.
But if you think that keeping sqlString is a better way, I can also modify the code.
In addition, there are some differences between the description form of spark here, for example, the StringLiteral of spark does not have quotation marks. If you think that this should be the same as the behavior of spark, I can also modify the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-1006319328
I found a related trino community MR in the email. The current behavior of spark iceberg is used in this MR UT.
https://github.com/trinodb/trino/pull/10214
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-959766469
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742220523
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
Same note as above.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -39,6 +39,7 @@
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.internal.ExactComparisonCriteria;
Review comment:
Is it normal to use this class that's got `internal` in the package name? Do we have to worry about behavioral changes if we upgrade JUnit?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -659,17 +658,7 @@ public String or(String leftResult, String rightResult) {
}
private static <T> String sqlString(List<org.apache.iceberg.expressions.Literal<T>> literals) {
- return literals.stream().map(DescribeExpressionVisitor::sqlString).collect(Collectors.joining(", "));
- }
-
- private static String sqlString(org.apache.iceberg.expressions.Literal<?> lit) {
- if (lit.value() instanceof String) {
- return "'" + lit.value() + "'";
- } else if (lit.value() instanceof ByteBuffer) {
- throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
- } else {
- return lit.value().toString();
- }
+ return literals.stream().map(Object::toString).collect(Collectors.joining(", "));
Review comment:
This seems possibly incorrect or a deviation from the behavior before.
It looks like this code is calling Object.toString on `Literal<T>`, whereas before, the `toString` call came from `Literal<T>::value`. Is that the intended change?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`).
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
I'm not sure if we have any other places where we convert a `ByteBufer` to a string.
If there are any, it seems like potentially we should add the stringify function to the `ByteBuffer` utility class and then try to use it uniformly everywhere. You don't necessarily need to update the other places to use them in this PR, but would you mind taking a look and seeing if you find any?
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
Follow up question: Are there cases where people might not use base16 for the representation or where the leading zero might not be used? I don't think so for Spark, but I'm less sure with Flink for example.
For Spark, looking at the documentation, it seems like it's just `X`, without the leading zero: https://spark.apache.org/docs/latest/sql-ref-literals.html#binary-literal
I'm not as sure about Trino, Flink, and other systems though.
For Trino, it looks to also start with the capital `X` (without the zero): https://trino.io/docs/current/language/types.html#varbinary
Looking it up, for Flink, it looks like maybe it's actually represented as `BINARY(n)` or `VARBINARY(n)`. This is the one I'm least sure about: https://nightlies.apache.org/flink/flink-docs-release-1.13/docs/dev/table/types/#binary
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -599,6 +600,13 @@ public String toString() {
Object writeReplace() throws ObjectStreamException {
return new SerializationProxies.FixedLiteralProxy(value());
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
There's an existing ByteBuffers utility class, org.apache.iceberg.util.ByteBuffers, which you can use here. Can you please use that for consistency? It's also more efficient in some cases (doesn't always necessarily allocate, handles offsets more thoroughly, etc).
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
I'd also add that repurposing existing test cases makes it harder for people who maintain forks and might not cherry-pick every commit.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
Same note as above.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -39,6 +39,7 @@
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.internal.ExactComparisonCriteria;
Review comment:
Is it normal to use this class that's got `internal` in the package name? Do we have to worry about behavioral changes if we upgrade JUnit?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -659,17 +658,7 @@ public String or(String leftResult, String rightResult) {
}
private static <T> String sqlString(List<org.apache.iceberg.expressions.Literal<T>> literals) {
- return literals.stream().map(DescribeExpressionVisitor::sqlString).collect(Collectors.joining(", "));
- }
-
- private static String sqlString(org.apache.iceberg.expressions.Literal<?> lit) {
- if (lit.value() instanceof String) {
- return "'" + lit.value() + "'";
- } else if (lit.value() instanceof ByteBuffer) {
- throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
- } else {
- return lit.value().toString();
- }
+ return literals.stream().map(Object::toString).collect(Collectors.joining(", "));
Review comment:
This seems possibly incorrect or a deviation from the behavior before.
It looks like this code is calling Object.toString on `Literal<T>`, whereas before, the `toString` call came from `Literal<T>::value`. Is that the intended change?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`).
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
I'm not sure if we have any other places where we convert a `ByteBufer` to a string.
If there are any, it seems like potentially we should add the stringify function to the `ByteBuffer` utility class and then try to use it uniformly everywhere. You don't necessarily need to update the other places to use them in this PR, but would you mind taking a look and seeing if you find any?
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
Follow up question: Are there cases where people might not use base16 for the representation or where the leading zero might not be used? I don't think so for Spark, but I'm less sure with Flink for example.
For Spark, looking at the documentation, it seems like it's just `X`, without the leading zero: https://spark.apache.org/docs/latest/sql-ref-literals.html#binary-literal
I'm not as sure about Trino, Flink, and other systems though.
For Trino, it looks to also start with the capital `X` (without the zero): https://trino.io/docs/current/language/types.html#varbinary
Looking it up, for Flink, it looks like maybe it's actually represented as `BINARY(n)` or `VARBINARY(n)`. This is the one I'm least sure about: https://nightlies.apache.org/flink/flink-docs-release-1.13/docs/dev/table/types/#binary
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -599,6 +600,13 @@ public String toString() {
Object writeReplace() throws ObjectStreamException {
return new SerializationProxies.FixedLiteralProxy(value());
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
There's an existing ByteBuffers utility class, org.apache.iceberg.util.ByteBuffers, which you can use here. Can you please use that for consistency? It's also more efficient in some cases (doesn't always necessarily allocate, handles offsets more thoroughly, etc).
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
I'd also add that repurposing existing test cases makes it harder for people who maintain forks and might not cherry-pick every commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742232207
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742220523
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
Same note as above.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -39,6 +39,7 @@
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.internal.ExactComparisonCriteria;
Review comment:
Is it normal to use this class that's got `internal` in the package name? Do we have to worry about behavioral changes if we upgrade JUnit?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -659,17 +658,7 @@ public String or(String leftResult, String rightResult) {
}
private static <T> String sqlString(List<org.apache.iceberg.expressions.Literal<T>> literals) {
- return literals.stream().map(DescribeExpressionVisitor::sqlString).collect(Collectors.joining(", "));
- }
-
- private static String sqlString(org.apache.iceberg.expressions.Literal<?> lit) {
- if (lit.value() instanceof String) {
- return "'" + lit.value() + "'";
- } else if (lit.value() instanceof ByteBuffer) {
- throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
- } else {
- return lit.value().toString();
- }
+ return literals.stream().map(Object::toString).collect(Collectors.joining(", "));
Review comment:
This seems possibly incorrect or a deviation from the behavior before.
It looks like this code is calling Object.toString on `Literal<T>`, whereas before, the `toString` call came from `Literal<T>::value`. Is that the intended change?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`).
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
I'm not sure if we have any other places where we convert a `ByteBufer` to a string.
If there are any, it seems like potentially we should add the stringify function to the `ByteBuffer` utility class and then try to use it uniformly everywhere. You don't necessarily need to update the other places to use them in this PR, but would you mind taking a look and seeing if you find any?
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
Follow up question: Are there cases where people might not use base16 for the representation or where the leading zero might not be used? I don't think so for Spark, but I'm less sure with Flink for example.
For Spark, looking at the documentation, it seems like it's just `X`, without the leading zero: https://spark.apache.org/docs/latest/sql-ref-literals.html#binary-literal
I'm not as sure about Trino, Flink, and other systems though.
For Trino, it looks to also start with the capital `X` (without the zero): https://trino.io/docs/current/language/types.html#varbinary
Looking it up, for Flink, it looks like maybe it's actually represented as `BINARY(n)` or `VARBINARY(n)`. This is the one I'm least sure about: https://nightlies.apache.org/flink/flink-docs-release-1.13/docs/dev/table/types/#binary
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -599,6 +600,13 @@ public String toString() {
Object writeReplace() throws ObjectStreamException {
return new SerializationProxies.FixedLiteralProxy(value());
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
There's an existing ByteBuffers utility class, org.apache.iceberg.util.ByteBuffers, which you can use here. Can you please use that for consistency? It's also more efficient in some cases (doesn't always necessarily allocate, handles offsets more thoroughly, etc).
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`)?
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
I'd also add that repurposing existing test cases makes it harder for people who maintain forks and might not cherry-pick every commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742301887
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/sql/TestSelect.java
##########
@@ -49,8 +49,8 @@ public TestSelect(String catalogName, String implementation, Map<String, String>
@Before
public void createTables() {
- 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'11'), (3, 'c', float('NaN'), X'1111')", tableName);
Review comment:
I'd also add that repurposing existing test cases makes it harder for people who maintain forks and might not cherry-pick every commit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742283725
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -157,7 +158,11 @@ private void assertEquals(String context, Object[] expectedRow, Object[] actualR
Object actualValue = actualRow[col];
if (expectedValue != null && expectedValue.getClass().isArray()) {
String newContext = String.format("%s (nested col %d)", context, col + 1);
- assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ if (expectedValue.getClass().getComponentType().isPrimitive()) {
+ new ExactComparisonCriteria().arrayEquals(newContext, expectedValue, actualValue);
+ } else {
+ assertEquals(newContext, (Object[]) expectedValue, (Object[]) actualValue);
+ }
Review comment:
Why was this needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-959766469
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r750613569
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -659,17 +658,7 @@ public String or(String leftResult, String rightResult) {
}
private static <T> String sqlString(List<org.apache.iceberg.expressions.Literal<T>> literals) {
- return literals.stream().map(DescribeExpressionVisitor::sqlString).collect(Collectors.joining(", "));
- }
-
- private static String sqlString(org.apache.iceberg.expressions.Literal<?> lit) {
- if (lit.value() instanceof String) {
- return "'" + lit.value() + "'";
- } else if (lit.value() instanceof ByteBuffer) {
- throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
- } else {
- return lit.value().toString();
- }
+ return literals.stream().map(Object::toString).collect(Collectors.joining(", "));
Review comment:
Thanks for your review. After testing, I think this is correct
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r781818876
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -118,6 +119,8 @@ protected long waitUntilAfter(long timestampMillis) {
return row.getList(pos);
} 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();
Review comment:
I think this is correct. The `toJava` method assumes that the elements of the returned array are all wrapper types, which is a false assumption. Spark's java api result has the potential to return an array of primitive types.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen closed pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen closed pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r748903035
##########
File path: api/src/main/java/org/apache/iceberg/util/ByteBuffers.java
##########
@@ -58,6 +59,11 @@ public static ByteBuffer copy(ByteBuffer buffer) {
return ByteBuffer.wrap(copyArray);
}
+ public static String encodeHexString(ByteBuffer buffer) {
+ byte[] bytes = toByteArray(buffer.duplicate());
Review comment:
No need for `duplicate` here because `toByteArray` won't modify the buffer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-970629936
Sorry for the late reply. I left some feedback. Thank you for your review @kbendick @rdblue ！
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-988606276
@rdblue @kbendick Could you help review continue? 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] izchen commented on pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
izchen commented on pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#issuecomment-959799737
@rdblue @kbendick , could you help to review this PR? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] kbendick commented on a change in pull request #3460: Spark: Fix IllegarlArgumentException when filtering on BinaryType column
Posted by GitBox <gi...@apache.org>.
kbendick commented on a change in pull request #3460:
URL: https://github.com/apache/iceberg/pull/3460#discussion_r742220523
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
Same note as above.
##########
File path: spark/v3.2/spark/src/test/java/org/apache/iceberg/spark/SparkTestBase.java
##########
@@ -39,6 +39,7 @@
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.BeforeClass;
+import org.junit.internal.ExactComparisonCriteria;
Review comment:
Is it normal to use this class that's got `internal` in the package name? Do we have to worry about behavioral changes if we upgrade JUnit?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -659,17 +658,7 @@ public String or(String leftResult, String rightResult) {
}
private static <T> String sqlString(List<org.apache.iceberg.expressions.Literal<T>> literals) {
- return literals.stream().map(DescribeExpressionVisitor::sqlString).collect(Collectors.joining(", "));
- }
-
- private static String sqlString(org.apache.iceberg.expressions.Literal<?> lit) {
- if (lit.value() instanceof String) {
- return "'" + lit.value() + "'";
- } else if (lit.value() instanceof ByteBuffer) {
- throw new IllegalArgumentException("Cannot convert bytes to SQL literal: " + lit);
- } else {
- return lit.value().toString();
- }
+ return literals.stream().map(Object::toString).collect(Collectors.joining(", "));
Review comment:
This seems possibly incorrect or a deviation from the behavior before.
It looks like this code is calling Object.toString on `Literal<T>`, whereas before, the `toString` call came from `Literal<T>::value`. Is that the intended change?
##########
File path: spark/v3.2/spark/src/main/java/org/apache/iceberg/spark/Spark3Util.java
##########
@@ -636,17 +635,17 @@ public String or(String leftResult, String rightResult) {
case NOT_NAN:
return "not_nan(" + pred.ref().name() + ")";
case LT:
- return pred.ref().name() + " < " + sqlString(pred.literal());
+ return pred.ref().name() + " < " + pred.literal();
case LT_EQ:
- return pred.ref().name() + " <= " + sqlString(pred.literal());
+ return pred.ref().name() + " <= " + pred.literal();
case GT:
- return pred.ref().name() + " > " + sqlString(pred.literal());
+ return pred.ref().name() + " > " + pred.literal();
case GT_EQ:
- return pred.ref().name() + " >= " + sqlString(pred.literal());
+ return pred.ref().name() + " >= " + pred.literal();
case EQ:
- return pred.ref().name() + " = " + sqlString(pred.literal());
+ return pred.ref().name() + " = " + pred.literal();
case NOT_EQ:
- return pred.ref().name() + " != " + sqlString(pred.literal());
+ return pred.ref().name() + " != " + pred.literal();
Review comment:
Why did we get rid of the `sqlString` function here? Is there a change that removes the need to quote strings for example?
Also, since the representation of `ByteBuffer` is potentially engine specific, would it be better to add the conversion to the `sqlString` here for Spark (e.g. using the leading capital X, like `X'123456'`).
This would allow other engines to handle it themselves (e.g. if Flink doesn't use leading `X` format).
I worry about unintended side effects from removing the explicit quoting as I've seen more PRs come up that handle adding and removing quotes as needed, but possibly you're more familiar with those conversations than I am.
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
I'm not sure if we have any other places where we convert a `ByteBufer` to a string.
If there are any, it seems like potentially we should add the stringify function to the `ByteBuffer` utility class and then try to use it uniformly everywhere. You don't necessarily need to update the other places to use them in this PR, but would you mind taking a look and seeing if you find any?
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -639,5 +647,12 @@ Object writeReplace() throws ObjectStreamException {
protected Type.TypeID typeId() {
return Type.TypeID.BINARY;
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
+ return "0x" + BaseEncoding.base16().encode(binary);
Review comment:
Follow up question: Are there cases where people might not use base16 for the representation or where the leading zero might not be used? I don't think so for Spark, but I'm less sure with Flink for example.
For Spark, looking at the documentation, it seems like it's just `X`, without the leading zero: https://spark.apache.org/docs/latest/sql-ref-literals.html#binary-literal
I'm not as sure about Trino, Flink, and other systems though.
For Trino, it looks to also start with the capital `X` (without the zero): https://trino.io/docs/current/language/types.html#varbinary
Looking it up, for Flink, it looks like maybe it's actually represented as `BINARY(n)` or `VARBINARY(n)`. This is the one I'm least sure about: https://nightlies.apache.org/flink/flink-docs-release-1.13/docs/dev/table/types/#binary
##########
File path: api/src/main/java/org/apache/iceberg/expressions/Literals.java
##########
@@ -599,6 +600,13 @@ public String toString() {
Object writeReplace() throws ObjectStreamException {
return new SerializationProxies.FixedLiteralProxy(value());
}
+
+ @Override
+ public String toString() {
+ byte[] binary = new byte[value().remaining()];
+ value().duplicate().get(binary);
Review comment:
There's an existing ByteBuffers utility class, org.apache.iceberg.util.ByteBuffers, which you can use here. Can you please use that for consistency? It's also more efficient in some cases (doesn't always necessarily allocate, handles offsets more thoroughly, etc).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org