You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "huaxingao (via GitHub)" <gi...@apache.org> on 2025/10/16 16:29:13 UTC
[PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
huaxingao opened a new pull request, #14349:
URL: https://github.com/apache/iceberg/pull/14349
(no comment)
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "aihuaxu (via GitHub)" <gi...@apache.org>.
aihuaxu commented on code in PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#discussion_r2482002613
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkVariantRead.java:
##########
@@ -169,4 +161,118 @@ public void testVariantNullValueProjection(boolean vectorized) {
assertThat(row.getLong(0)).isEqualTo(10L);
assertThat(row.isNullAt(1)).isTrue();
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testNestedStructVariant(boolean vectorized) {
+ assumeThat(vectorized).as("Variant vectorized Parquet read is not implemented yet").isFalse();
+ setVectorization(vectorized);
+
+ String structTable = CATALOG + ".default.var_struct";
+ sql("DROP TABLE IF EXISTS %s", structTable);
+ sql(
+ "CREATE TABLE %s (id BIGINT, s STRUCT<v: VARIANT>) USING iceberg "
+ + "TBLPROPERTIES ('format-version'='3', 'read.parquet.vectorization.enabled'='false')",
+ structTable);
+
+ String j1 = "{\"a\":1}";
+ String j2 = "{\"b\":2}";
+ sql("INSERT INTO %s SELECT 1, named_struct('v', parse_json('%s'))", structTable, j1);
+ sql("INSERT INTO %s SELECT 2, named_struct('v', parse_json('%s'))", structTable, j2);
+
+ Dataset<Row> df =
+ spark.table(structTable).selectExpr("id", "to_json(s.v) as v_json").orderBy("id");
+ java.util.List<Row> rows = df.collectAsList();
+ assertThat(rows.get(0).getLong(0)).isEqualTo(1L);
+ assertThat(rows.get(0).getString(1)).isEqualTo("{\"a\":1}");
Review Comment:
Same for the following tests.
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkVariantRead.java:
##########
@@ -169,4 +161,118 @@ public void testVariantNullValueProjection(boolean vectorized) {
assertThat(row.getLong(0)).isEqualTo(10L);
assertThat(row.isNullAt(1)).isTrue();
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testNestedStructVariant(boolean vectorized) {
+ assumeThat(vectorized).as("Variant vectorized Parquet read is not implemented yet").isFalse();
+ setVectorization(vectorized);
+
+ String structTable = CATALOG + ".default.var_struct";
+ sql("DROP TABLE IF EXISTS %s", structTable);
+ sql(
+ "CREATE TABLE %s (id BIGINT, s STRUCT<v: VARIANT>) USING iceberg "
+ + "TBLPROPERTIES ('format-version'='3', 'read.parquet.vectorization.enabled'='false')",
+ structTable);
+
+ String j1 = "{\"a\":1}";
+ String j2 = "{\"b\":2}";
+ sql("INSERT INTO %s SELECT 1, named_struct('v', parse_json('%s'))", structTable, j1);
+ sql("INSERT INTO %s SELECT 2, named_struct('v', parse_json('%s'))", structTable, j2);
+
+ Dataset<Row> df =
+ spark.table(structTable).selectExpr("id", "to_json(s.v) as v_json").orderBy("id");
+ java.util.List<Row> rows = df.collectAsList();
+ assertThat(rows.get(0).getLong(0)).isEqualTo(1L);
+ assertThat(rows.get(0).getString(1)).isEqualTo("{\"a\":1}");
Review Comment:
Why do we get the string here? I think we should get variant and then extract variant field 'a' value which is expected to be 1.
Same for line#189.
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#discussion_r2479078090
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkVariantRead.java:
##########
@@ -169,4 +169,112 @@ public void testVariantNullValueProjection(boolean vectorized) {
assertThat(row.getLong(0)).isEqualTo(10L);
assertThat(row.isNullAt(1)).isTrue();
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testNestedStructVariant(boolean vectorized) {
+ assumeThat(vectorized).as("Variant vectorized Parquet read is not implemented yet").isFalse();
+ spark.conf().set("spark.sql.parquet.enableVectorizedReader", String.valueOf(vectorized));
Review Comment:
I just noticed that some of the tests in this test set vectorization via table property. To be consistent, I will change all the tests to use table property.
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#discussion_r2482968216
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkVariantRead.java:
##########
@@ -169,4 +161,118 @@ public void testVariantNullValueProjection(boolean vectorized) {
assertThat(row.getLong(0)).isEqualTo(10L);
assertThat(row.isNullAt(1)).isTrue();
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testNestedStructVariant(boolean vectorized) {
+ assumeThat(vectorized).as("Variant vectorized Parquet read is not implemented yet").isFalse();
+ setVectorization(vectorized);
+
+ String structTable = CATALOG + ".default.var_struct";
+ sql("DROP TABLE IF EXISTS %s", structTable);
+ sql(
+ "CREATE TABLE %s (id BIGINT, s STRUCT<v: VARIANT>) USING iceberg "
+ + "TBLPROPERTIES ('format-version'='3', 'read.parquet.vectorization.enabled'='false')",
+ structTable);
+
+ String j1 = "{\"a\":1}";
+ String j2 = "{\"b\":2}";
+ sql("INSERT INTO %s SELECT 1, named_struct('v', parse_json('%s'))", structTable, j1);
+ sql("INSERT INTO %s SELECT 2, named_struct('v', parse_json('%s'))", structTable, j2);
+
+ Dataset<Row> df =
+ spark.table(structTable).selectExpr("id", "to_json(s.v) as v_json").orderBy("id");
+ java.util.List<Row> rows = df.collectAsList();
+ assertThat(rows.get(0).getLong(0)).isEqualTo(1L);
+ assertThat(rows.get(0).getString(1)).isEqualTo("{\"a\":1}");
Review Comment:
Fixed Thanks!
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#issuecomment-3439493033
cc @aihuaxu @amogh-jahagirdar @nastra @singhpk234
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on code in PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#discussion_r2464399429
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java:
##########
@@ -776,7 +776,7 @@ public MapData getMap(int ordinal) {
@Override
public VariantVal getVariant(int ordinal) {
- throw new UnsupportedOperationException("Unsupported method: getVariant");
+ return (VariantVal) values[ordinal];
Review Comment:
do we need a Precondition before case for error message ?
##########
spark/v4.0/spark/src/test/java/org/apache/iceberg/spark/sql/TestSparkVariantRead.java:
##########
@@ -169,4 +169,112 @@ public void testVariantNullValueProjection(boolean vectorized) {
assertThat(row.getLong(0)).isEqualTo(10L);
assertThat(row.isNullAt(1)).isTrue();
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {false, true})
+ public void testNestedStructVariant(boolean vectorized) {
+ assumeThat(vectorized).as("Variant vectorized Parquet read is not implemented yet").isFalse();
+ spark.conf().set("spark.sql.parquet.enableVectorizedReader", String.valueOf(vectorized));
Review Comment:
nit : we have withSQLConf in iceberg too, can try using it ?
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao merged PR #14349:
URL: https://github.com/apache/iceberg/pull/14349
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#discussion_r2479074715
##########
spark/v4.0/spark/src/main/java/org/apache/iceberg/spark/data/SparkParquetReaders.java:
##########
@@ -776,7 +776,7 @@ public MapData getMap(int ordinal) {
@Override
public VariantVal getVariant(int ordinal) {
- throw new UnsupportedOperationException("Unsupported method: getVariant");
+ return (VariantVal) values[ordinal];
Review Comment:
I think it can only be `VariantVal` here. Also other methods in this class don't have a precondition check. I prefer to be consistent with other methods.
--
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
Re: [PR] Spark: Implement ArrayData.getVariant for row-based Parquet readers [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on PR #14349:
URL: https://github.com/apache/iceberg/pull/14349#issuecomment-3488758100
Thanks @aihuaxu @singhpk234 for the review!
--
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