You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rocco408 (via GitHub)" <gi...@apache.org> on 2024/10/29 06:15:54 UTC
[PR] Ignore schema merge updates from long -> int [iceberg]
rocco408 opened a new pull request, #11419:
URL: https://github.com/apache/iceberg/pull/11419
This PR attempt to address https://github.com/apache/iceberg/issues/4849.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1821080741
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +184,14 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Types.NestedField field, Types.NestedField existingField) {
+ switch (existingField.type().typeId()) {
+ case LONG:
Review Comment:
We should also accept FLOAT == DOUBLE 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
Re: [PR] Ignore schema merge updates from long -> int [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1834751371
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
Shall we also consider DecimalType with different precision?
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1830240450
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
Review Comment:
Thanks for calling this out, I verified it indeed fails when asserting against rows with the int variant like `row(1, "a")` so I think it doing the right thing.
I'll assert on the schema as well 👍
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839833974
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +230,132 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDoubleToFloat() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id double, data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> doubleDF =
+ jsonToDF(
+ "id double, data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ doubleDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with double column",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> floatDF =
+ jsonToDF(
+ "id float, data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged double column type",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b"), row(3.0, "c"), row(4.0, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.DOUBLE));
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDecimalToDecimalWithNarrowerPrecision() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id decimal(6,2), data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> decimalPrecision6DF =
+ jsonToDF(
+ "id decimal(6,2), data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ decimalPrecision6DF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with decimal column with precision 6",
+ ImmutableList.of(row(new BigDecimal("1.00"), "a"), row(new BigDecimal("2.00"), "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> decimalPrecision4DF =
+ jsonToDF(
+ "id decimal(4,2), data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(
+ () -> decimalPrecision4DF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged decimal precision",
+ ImmutableList.of(
+ row(new BigDecimal("1.00"), "a"),
+ row(new BigDecimal("2.00"), "b"),
+ row(new BigDecimal("3.00"), "c"),
+ row(new BigDecimal("4.00"), "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the decimal column precision did not change
+ Type idFieldType =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id").type();
+ assertThat(idFieldType.typeId().equals(Type.TypeID.DECIMAL));
Review Comment:
```suggestion
assertThat(idFieldType.typeId()).isEqualTo(Type.TypeID.DECIMAL);
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839878542
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
+ Schema currentSchema = new Schema(required(1, "aCol", LongType.get()));
+ Schema newSchema = new Schema(required(1, "aCol", IntegerType.get()));
+
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(LongType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteDecimalToNarrowerPrecision() {
Review Comment:
```suggestion
public void testDecimalToNarrowerPrecision() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839923060
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
Review Comment:
the test seems to pass independent of whether this is set or not
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
Review Comment:
the test seems to pass independent of whether this is set or not
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1827997047
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -163,8 +163,7 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
String fullName = partnerSchema.findColumnName(existingField.fieldId());
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
- boolean needsTypeUpdate =
- field.type().isPrimitiveType() && !field.type().equals(existingField.type());
+ boolean needsTypeUpdate = !ignorableUpdate(field.type(), existingField.type());
Review Comment:
nit: But we could flip the sign in the method and ignore the negation. Also we could match names 🤷
```
boolean needsTypeUpdate = needsTypeUpdate(field.type(), existingField.type());
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1828001098
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
Review Comment:
Probably should just be two separate tests, we could do it as a parameterized test as well if you like.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2466456512
@rocco408 almost there, it looks like there are some import errors:
```
/home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:256: error: cannot find symbol
> Task :iceberg-spark:iceberg-spark-3.3_2.12:compileTestJava
assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
^
symbol: method assertThatCode(()->intDF.[...]end())
location: class TestDataFrameWriterV2
/home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:267: error: cannot find symbol
assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
^
symbol: method assertThat(boolean)
location: class TestDataFrameWriterV2
/home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:298: error: cannot find symbol
assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
^
symbol: method assertThatCode(()->floatD[...]end())
location: class TestDataFrameWriterV2
/home/runner/work/iceberg/iceberg/spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:309: error: cannot find symbol
assertThat(idField.type().typeId().equals(Type.TypeID.DOUBLE));
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1834604654
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
I've tried different variations and it consistently tells me `Conditional logic can be removed. [SimplifyBooleanReturn]` on this statement `if (existingType.typeId() == Type.TypeID.DOUBLE && newType.typeId() == Type.TypeID.FLOAT)`
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1834599748
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
```shell
$ ./gradlew build -x test -x integrationTest
Configuration on demand is an incubating feature.
> Task :iceberg-open-api:validateRESTCatalogSpec
Validating spec /Users/rocco/Github/iceberg/open-api/rest-catalog-open-api.yaml
Spec is valid.
> Task :iceberg-aws:validateS3SignerSpec
Validating spec /Users/rocco/Github/iceberg/aws/src/main/resources/s3-signer-open-api.yaml
Spec is valid.
> Task :iceberg-core:checkstyleMain
[ant:checkstyle] [ERROR] /Users/rocco/Github/iceberg/core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:189:14: Conditional logic can be removed. [SimplifyBooleanReturn]
> Task :iceberg-core:checkstyleMain FAILED
FAILURE: Build failed with an exception.
* What went wrong:
Execution failed for task ':iceberg-core:checkstyleMain'.
> A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction
> Checkstyle rule violations were found. See the report at: file:///Users/rocco/Github/iceberg/core/build/reports/checkstyle/main.html
Checkstyle files with violations: 1
Checkstyle violations by severity: [error:1]
* Try:
> Run with --scan to get full insights.
Deprecated Gradle features were used in this build, making it incompatible with Gradle 9.0.
You can use '--warning-mode all' to show the individual deprecation warnings and determine if they come from your own scripts or plugins.
For more on this, please refer to https://docs.gradle.org/8.10.2/userguide/command_line_interface.html#sec:command_line_warnings in the Gradle documentation.
BUILD FAILED in 12s
422 actionable tasks: 25 executed, 2 from cache, 395 up-to-date
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1835074284
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
https://github.com/rocco408/iceberg/pull/1/files - Futzed around with this and I think this may be cleaner. I ended up renegating the function and used a switch statement. When I think about adding the decimal case, this seems like it will be cleaner
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1835224007
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
Thank you @RussellSpitzer and @huaxingao, I pulled in those changes 🙇
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2468651554
I think maybe the compatibleType name may need to change ... just because we sometimes just allow invalid types to go through so they can fail later ...
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2468641393
Didn't understand your comment @rocco408 , We were just missing that any NonPrimitive -> NonPrimitive are considered compatible.
https://github.com/rocco408/iceberg/pull/3/files
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2466841180
I may be missing something, but after several variations I don't see a way to cleanly use `TypeUtil.isPromotionAllowed` from `UnionByNameVisitor`. Even after adding the long-to-int and double-to-float case in `TypeUtil.isPromotionAllowed` like this
```shell
--- a/api/src/main/java/org/apache/iceberg/types/TypeUtil.java
+++ b/api/src/main/java/org/apache/iceberg/types/TypeUtil.java
@@ -406,9 +406,15 @@ public class TypeUtil {
case INTEGER:
return to.typeId() == Type.TypeID.LONG;
+ case LONG:
+ return to.typeId() != Type.TypeID.INTEGER;
+
case FLOAT:
return to.typeId() == Type.TypeID.DOUBLE;
+ case DOUBLE:
+ return to.typeId() != Type.TypeID.FLOAT;
+
```
and adding special handling for non-primitive types in `UnionByNameVisitor` like this
```shell
--- a/core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java
+++ b/core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java
@@ -164,7 +164,7 @@ public class UnionByNameVisitor extends SchemaWithPartnerVisitor<Integer, Boolea
String fullName = partnerSchema.findColumnName(existingField.fieldId());
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
- boolean needsTypeUpdate = !compatibleType(field.type(), existingField.type());
+ boolean needsTypeUpdate = needsTypeUpdate(field.type(), existingField.type());
boolean needsDocUpdate = field.doc() != null && !field.doc().equals(existingField.doc());
if (needsOptionalUpdate) {
@@ -180,8 +180,11 @@ public class UnionByNameVisitor extends SchemaWithPartnerVisitor<Integer, Boolea
}
}
- private boolean compatibleType(Type newType, Type existingType) {
- return existingType.isPrimitiveType() && TypeUtil.isPromotionAllowed(newType, existingType.asPrimitiveType());
+ private boolean needsTypeUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType() && !existingType.isPrimitiveType()) {
+ return true;
+ }
+ return newType.isPrimitiveType() && !newType.equals(existingType) && TypeUtil.isPromotionAllowed(existingType, newType.asPrimitiveType());
}
```
I'm hitting failures like with [TestSchemaUnionByFieldName.testDetectInvalidTopLevelList](https://github.com/rocco408/iceberg/blob/fff9ec3bbc322080da6363b657415b039c0e92a0/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java#L242-L250) where it attempts to cast a list of strings to list of longs. By using `TypeUtil.isPromotionAllowed` in `UnionByNameVisitor` we end up ignoring this type of casting because it is not allowed. However in the existing flow, [needsTypeUpdate](https://github.com/rocco408/iceberg/blob/fff9ec3bbc322080da6363b657415b039c0e92a0/core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java#L174-L176) ends up getting set to `true` where we expect an exception to eventually be thrown when we use `TypeUtil.isPromotionAllowed` [here](https://github.com/rocco408/iceberg/blob/fff9ec3bbc322080da6363b657415b039c0e92a0/core/src/main/java/org/apache/iceberg/SchemaUpdate.java#L287-L292).
I think this ignore behavior needs to be isolated to `UnionByNameVisitor`. As always I'm open to other ideas if I missed something.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1834980539
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
i bet it wants you to smash that into the previous predicate. But i'm not sure that would be clearer for reading.
ie:
((double && float) || (long && int))
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1828013629
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ try {
+ intDF.writeTo(tableName).option("merge-schema", "true").append();
+ } catch (IllegalArgumentException e) {
+ Assert.fail(String.format("Should not throw exception: %s", e));
+ }
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ String[] tblNameSplit = tableName.split("\\.");
+ String tblIdentifier =
+ tblNameSplit.length == 3
+ ? String.format("%s.%s", tblNameSplit[1], tblNameSplit[2])
+ : tableName;
+ Table table = validationCatalog.loadTable(TableIdentifier.parse(tblIdentifier));
+ Types.NestedField idField = table.schema().findField("id");
+ Assert.assertEquals("Column type should not change", idField.type().typeId(), Type.TypeID.LONG);
+
+ //// test double to float ignore case
Review Comment:
New Test here (or parameterize .. probably won't be worth it to parametrize)
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2471220679
Thanks again Russ, and Huaxin. I see where my previous attempt failed. In my last attempt I had swapped the placement of params to this `isPromotionAllowed(existingType, newType.asPrimitiveType())` which led me to update that method as well. Anywho, I understand what we're doing now, we're basically checking whether the `existingType` is wider then the `newType`, in which case we say we're compatible which results in ignoring an update. I'll tidy things up momentarily.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839878112
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
Review Comment:
```suggestion
public void testPromoteLongToInt() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839926221
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
Review Comment:
please also update all the other tests that are affected
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2475221160
Thanks @nastra, I think I addressed all your suggestions, cheers.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1827997047
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -163,8 +163,7 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
String fullName = partnerSchema.findColumnName(existingField.fieldId());
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
- boolean needsTypeUpdate =
- field.type().isPrimitiveType() && !field.type().equals(existingField.type());
+ boolean needsTypeUpdate = !ignorableUpdate(field.type(), existingField.type());
Review Comment:
nit: But we could flip the sign in the method and ignore the negation.
```
boolean needsTypeUpdate = needsTypeUpdate(field.type(), existingField.type());
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1833914062
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
Thanks for the suggestion, but I'm getting checkstyle rule violations when adding `else if`. This is what I tried, which appears to be logically correct
```java
if (newType.isPrimitiveType()) {
if (newType.equals(existingType)) {
return false;
} else if (existingType.typeId() == Type.TypeID.LONG
&& newType.typeId() == Type.TypeID.INTEGER) {
return false;
} else if (existingType.typeId() == Type.TypeID.DOUBLE
&& newType.typeId() == Type.TypeID.FLOAT) {
return false;
} else {
return true;
}
}
return false;
```
This is what I'm inclined to go with unless you have strong opinions on an alternative.
```java
if (newType.isPrimitiveType()) {
if (newType.equals(existingType)) {
return false;
}
if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
return false;
}
if (existingType.typeId() == Type.TypeID.DOUBLE && newType.typeId() == Type.TypeID.FLOAT) {
return false;
}
return true;
}
return false;
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2466513581
This latest change to using TypeUtil.isPromotionAllowed() is failing a number of tests in TestSchemaUnionByFieldName. Further attempts to modify TypeUtil.isPromotionAllowed() and it's usage from UnionByNameVisitor are breaking other things like expectations of non-primitives to primitive conversions to throw exceptions. I think the issue is that TypeUtil.isPromotionAllowed() allows some things like list<string> -> string where we expect exceptions to be thrown elsewhere. I don't think we can use TypeUtil.isPromotionAllowed() without making more widespread updates elsewhere.
I had actually considered updating TypeUtil.isPromotionAllowed() When I originally looked at this issue then decided not to touch it because of this reason.
Open to thoughts whenever folks have time.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839922654
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
Review Comment:
the test seems to pass independent of whether is is set or not
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1838575824
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
+ }
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingDoubleToFloat() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id double, data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> doubleDF =
+ jsonToDF(
+ "id double, data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ doubleDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with double column",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> floatDF =
+ jsonToDF(
+ "id float, data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged double column type",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b"), row(3.0, "c"), row(4.0, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.DOUBLE));
+ }
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingDecimalToDecimalWithNarrowerPrecision() throws Exception {
Review Comment:
I'm not in love with the length of this test but it's clear what it does 😅
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839893460
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,17 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean compatibleType(Type newType, Type existingType) {
+ if (existingType.isPrimitiveType()) {
+ // Primitive -> Wider Primitive
+ return newType.isPrimitiveType()
+ && TypeUtil.isPromotionAllowed(newType, existingType.asPrimitiveType());
Review Comment:
shouldn't this be `TypeUtil.isPromotionAllowed(existingType, newType.asPrimitiveType())`? The signature of the method is `public static boolean isPromotionAllowed(Type from, Type.PrimitiveType to)`
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839833296
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +230,132 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDoubleToFloat() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id double, data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> doubleDF =
+ jsonToDF(
+ "id double, data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ doubleDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with double column",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> floatDF =
+ jsonToDF(
+ "id float, data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged double column type",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b"), row(3.0, "c"), row(4.0, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.DOUBLE));
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDecimalToDecimalWithNarrowerPrecision() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id decimal(6,2), data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> decimalPrecision6DF =
+ jsonToDF(
+ "id decimal(6,2), data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ decimalPrecision6DF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with decimal column with precision 6",
+ ImmutableList.of(row(new BigDecimal("1.00"), "a"), row(new BigDecimal("2.00"), "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> decimalPrecision4DF =
+ jsonToDF(
+ "id decimal(4,2), data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(
+ () -> decimalPrecision4DF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged decimal precision",
+ ImmutableList.of(
+ row(new BigDecimal("1.00"), "a"),
+ row(new BigDecimal("2.00"), "b"),
+ row(new BigDecimal("3.00"), "c"),
+ row(new BigDecimal("4.00"), "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the decimal column precision did not change
+ Type idFieldType =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id").type();
+ assertThat(idFieldType.typeId().equals(Type.TypeID.DECIMAL));
+ Types.DecimalType decimalType = (Types.DecimalType) idFieldType;
+ assertThat(decimalType.precision() == 6);
Review Comment:
```suggestion
assertThat(decimalType.precision()).isEqualTo(6);
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1840549260
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,17 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean compatibleType(Type newType, Type existingType) {
+ if (existingType.isPrimitiveType()) {
+ // Primitive -> Wider Primitive
+ return newType.isPrimitiveType()
+ && TypeUtil.isPromotionAllowed(newType, existingType.asPrimitiveType());
Review Comment:
This is all my fault since I wrote those names in my suggestion in the first place, sorry for churn
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1829304556
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
Style nit:
```suggestion
} else if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
```
Since there is no fall-through possible.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1821107711
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -164,7 +167,8 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
boolean needsTypeUpdate =
- field.type().isPrimitiveType() && !field.type().equals(existingField.type());
+ (field.type().isPrimitiveType() && !field.type().equals(existingField.type()))
Review Comment:
I think we may just want to extract all of this logic now into a helper function since we already have some of it in ignorableUpdate. We also only probably need to pass through the types rather than the whole field.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1828008335
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
Review Comment:
I'm slightly worried this will be true regardless of the types. I think ... haven't tested this, that 1L and 1 will pass AssertEquals in our rows.
Probably we should also assert on schema
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1835050238
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
@huaxingao 's Suggestion I think is also a good one to consider, but let's do that in a followup because I feel less confident about that one. I think it's fine though
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1828011849
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ try {
+ intDF.writeTo(tableName).option("merge-schema", "true").append();
+ } catch (IllegalArgumentException e) {
+ Assert.fail(String.format("Should not throw exception: %s", e));
+ }
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ String[] tblNameSplit = tableName.split("\\.");
Review Comment:
May be easier to use the Spark Helpers here, although the validation catalog is also fine to check but I don't think it matters much in this case.
```java
Spark3Util.loadIcebergTable(spark, tableName).schema()
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839877526
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Review Comment:
```suggestion
public void testPromoteDoubleToFloat() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839878542
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
+ Schema currentSchema = new Schema(required(1, "aCol", LongType.get()));
+ Schema newSchema = new Schema(required(1, "aCol", IntegerType.get()));
+
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(LongType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteDecimalToNarrowerPrecision() {
Review Comment:
```suggestion
public void testDowncastDecimalToNarrowerPrecision() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839878112
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
Review Comment:
```suggestion
public void testDowncastLongToInt() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839877526
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Review Comment:
```suggestion
public void testDowncastDoubleToFloat() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839861453
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,17 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean compatibleType(Type newType, Type existingType) {
Review Comment:
```suggestion
private boolean isCompatibleType(Type existingType, Type newType) {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1841669608
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,23 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean isIgnorableTypeUpdate(Type existingType, Type newType) {
+ if (existingType.isPrimitiveType()) {
+ /* TypeUtil.isPromotionAllowed is used to check whether type promotion is allowed in the reverse order,
Review Comment:
thanks, the comment makes things much clearer. Could you please switch the comment style from /*...*/ to // as the project doesn't generally use /*...*/ for in-line comments?
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2475577208
thanks @rocco408, this is almost ready to go, just had a few more minor comments
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1828006209
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +229,100 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ //// test long to int ignore case
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ try {
Review Comment:
We are using assertJ which has a fun little wrapper for this
```java
assertThatCode(() -> 1 + 1).doesNotThrowAnyException();;
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1827994321
##########
api/src/main/java/org/apache/iceberg/UpdateSchema.java:
##########
@@ -369,7 +369,9 @@ default UpdateSchema updateColumn(String name, Type.PrimitiveType newType, Strin
* to create a union schema.
*
* <p>For fields with same canonical names in both schemas it is required that the widen types is
- * supported using {@link UpdateSchema#updateColumn(String, Type.PrimitiveType)}
+ * supported using {@link UpdateSchema#updateColumn(String, Type.PrimitiveType)}. We ignore
Review Comment:
Avoid first/second person in the javadocs.
```
Differences in type are ignored if the new type is narrower than the existing type ....
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1825187149
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +226,40 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreDowncast() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with bigint column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ // should not throw 'java.lang.IllegalArgumentException: Cannot change column type: id: long ->
+ // int'
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
Review Comment:
> Task :iceberg-spark:iceberg-spark-3.4_2.12:compileTestJava
^
symbol: method assertThatCode(()->intDF.[...]end())
location: class TestDataFrameWriterV2
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1841387395
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -163,8 +164,7 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
String fullName = partnerSchema.findColumnName(existingField.fieldId());
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
- boolean needsTypeUpdate =
- field.type().isPrimitiveType() && !field.type().equals(existingField.type());
+ boolean needsTypeUpdate = !compatibleType(field.type(), existingField.type());
Review Comment:
Good catch, we included this in the latest update, it's included in the helper method.
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839960587
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
Review Comment:
nvm. the test fails when `merge-schema` is set to `true` but your fix isn't applied. I think it would make sense to add a comment to make this clearer in this test
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
Review Comment:
the test seems to pass independent of whether this is set or not
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1841670685
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -209,4 +213,132 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId()).isEqualTo(Type.TypeID.LONG);
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDoubleToFloat() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id double, data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> doubleDF =
+ jsonToDF(
+ "id double, data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ doubleDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with double column",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> floatDF =
+ jsonToDF(
+ "id float, data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged double column type",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b"), row(3.0, "c"), row(4.0, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId()).isEqualTo(Type.TypeID.DOUBLE);
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDecimalToDecimalWithNarrowerPrecision() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id decimal(6,2), data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> decimalPrecision6DF =
+ jsonToDF(
+ "id decimal(6,2), data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ decimalPrecision6DF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with decimal column with precision 6",
+ ImmutableList.of(row(new BigDecimal("1.00"), "a"), row(new BigDecimal("2.00"), "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> decimalPrecision4DF =
+ jsonToDF(
+ "id decimal(4,2), data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(
+ () -> decimalPrecision4DF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged decimal precision",
+ ImmutableList.of(
+ row(new BigDecimal("1.00"), "a"),
+ row(new BigDecimal("2.00"), "b"),
+ row(new BigDecimal("3.00"), "c"),
+ row(new BigDecimal("4.00"), "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the decimal column precision did not change
+ Type idFieldType =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id").type();
+ assertThat(idFieldType.typeId()).isEqualTo(Type.TypeID.DECIMAL);
+ Types.DecimalType decimalType = (Types.DecimalType) idFieldType;
+ assertThat(decimalType.precision() == 6);
Review Comment:
this also needs to be updated
--
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] API, Core, Spark: Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #11419:
URL: https://github.com/apache/iceberg/pull/11419
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2468649457
I think we may need to change the name from compatibleType ... I think the
check here is more like "ignoreTypeChange" since we basically let invalid
type changes go through to fail in other parts of the code :shrug:
On Mon, Nov 11, 2024 at 12:12 AM Huaxin Gao ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java
> <https://github.com/apache/iceberg/pull/11419#discussion_r1836006381>:
>
> > @@ -163,8 +164,7 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
> String fullName = partnerSchema.findColumnName(existingField.fieldId());
>
> boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
> - boolean needsTypeUpdate =
> - field.type().isPrimitiveType() && !field.type().equals(existingField.type());
> + boolean needsTypeUpdate = !compatibleType(field.type(), existingField.type());
>
> Do we also need the field.type().isPrimitiveType() here?
>
> boolean needsTypeUpdate = field.type().isPrimitiveType() && !compatibleType(field.type(), existingField.type());
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/iceberg/pull/11419#pullrequestreview-2426195003>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AADE2YOSLV5WRENR2EZVP6T2ABDDBAVCNFSM6AAAAABQZCVRCOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMRWGE4TKMBQGM>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#issuecomment-2466544717
Isn't it only allowing this case that the new type is a primitive? I guess we need to check that both are primitives in the clause
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "rocco408 (via GitHub)" <gi...@apache.org>.
rocco408 commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1842672093
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,17 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean compatibleType(Type newType, Type existingType) {
+ if (existingType.isPrimitiveType()) {
+ // Primitive -> Wider Primitive
+ return newType.isPrimitiveType()
+ && TypeUtil.isPromotionAllowed(newType, existingType.asPrimitiveType());
Review Comment:
Fixed it last update
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "RussellSpitzer (via GitHub)" <gi...@apache.org>.
RussellSpitzer commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1834496730
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +179,21 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean ignorableUpdate(Type newType, Type existingType) {
+ if (newType.isPrimitiveType()) {
+ if (newType.equals(existingType)) {
+ return true;
+ }
+ if (existingType.typeId() == Type.TypeID.LONG && newType.typeId() == Type.TypeID.INTEGER) {
Review Comment:
What is the checkstyle error?
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "huaxingao (via GitHub)" <gi...@apache.org>.
huaxingao commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1836006381
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -163,8 +164,7 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
String fullName = partnerSchema.findColumnName(existingField.fieldId());
boolean needsOptionalUpdate = field.isOptional() && existingField.isRequired();
- boolean needsTypeUpdate =
- field.type().isPrimitiveType() && !field.type().equals(existingField.type());
+ boolean needsTypeUpdate = !compatibleType(field.type(), existingField.type());
Review Comment:
Do we also need the `field.type().isPrimitiveType()` here?
```
boolean needsTypeUpdate = field.type().isPrimitiveType() && !compatibleType(field.type(), existingField.type());
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839842253
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +230,132 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
Review Comment:
```suggestion
assertThat(idField.type().typeId()).isEqualTo(Type.TypeID.LONG);
```
##########
spark/v3.3/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -226,4 +230,132 @@ public void testWriteWithCaseSensitiveOption() throws NoSuchTableException, Pars
fields = Spark3Util.loadIcebergTable(sparkSession, tableName).schema().asStruct().fields();
Assert.assertEquals(4, fields.size());
}
+
+ @Test
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
+ }
+
+ @Test
+ public void testMergeSchemaIgnoreCastingDoubleToFloat() throws Exception {
+ removeTables();
+ sql("CREATE TABLE %s (id double, data string) USING iceberg", tableName);
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> doubleDF =
+ jsonToDF(
+ "id double, data string",
+ "{ \"id\": 1.0, \"data\": \"a\" }",
+ "{ \"id\": 2.0, \"data\": \"b\" }");
+
+ doubleDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with double column",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> floatDF =
+ jsonToDF(
+ "id float, data string",
+ "{ \"id\": 3.0, \"data\": \"c\" }",
+ "{ \"id\": 4.0, \"data\": \"d\" }");
+
+ assertThatCode(() -> floatDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged double column type",
+ ImmutableList.of(row(1.0, "a"), row(2.0, "b"), row(3.0, "c"), row(4.0, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.DOUBLE));
Review Comment:
same as above
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839877526
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Review Comment:
```suggestion
public void testDowncastDoubleToFloat() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839878112
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
Review Comment:
```suggestion
public void testDowncastLongToInt() {
```
##########
core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java:
##########
@@ -303,13 +303,33 @@ public void testTypePromoteFloatToDouble() {
}
@Test
- public void testInvalidTypePromoteDoubleToFloat() {
+ public void testIgnoreTypePromoteDoubleToFloat() {
Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
- assertThatThrownBy(() -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
- .isInstanceOf(IllegalArgumentException.class)
- .hasMessage("Cannot change column type: aCol: double -> float");
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct()).isEqualTo(currentSchema.asStruct());
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(DoubleType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteLongToInt() {
+ Schema currentSchema = new Schema(required(1, "aCol", LongType.get()));
+ Schema newSchema = new Schema(required(1, "aCol", IntegerType.get()));
+
+ Schema applied = new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+ assertThat(applied.asStruct().fields()).hasSize(1);
+ assertThat(applied.asStruct().fields().get(0).type()).isEqualTo(LongType.get());
+ }
+
+ @Test
+ public void testIgnoreTypePromoteDecimalToNarrowerPrecision() {
Review Comment:
```suggestion
public void testDowncastDecimalToNarrowerPrecision() {
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839924799
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
+ .doesNotThrowAnyException();
+
+ assertEquals(
+ "Should include new rows with unchanged long column type",
+ ImmutableList.of(row(1L, "a"), row(2L, "b"), row(3L, "c"), row(4L, "d")),
+ sql("select * from %s order by id", tableName));
+
+ // verify the column type did not change
+ Types.NestedField idField =
+ Spark3Util.loadIcebergTable(spark, tableName).schema().findField("id");
+ assertThat(idField.type().typeId().equals(Type.TypeID.LONG));
Review Comment:
```suggestion
assertThat(idField.type().typeId()).isEqualTo(Type.TypeID. LONG);
```
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1839935623
##########
core/src/main/java/org/apache/iceberg/schema/UnionByNameVisitor.java:
##########
@@ -180,6 +180,17 @@ private void updateColumn(Types.NestedField field, Types.NestedField existingFie
}
}
+ private boolean compatibleType(Type newType, Type existingType) {
+ if (existingType.isPrimitiveType()) {
+ // Primitive -> Wider Primitive
+ return newType.isPrimitiveType()
+ && TypeUtil.isPromotionAllowed(newType, existingType.asPrimitiveType());
Review Comment:
just reading https://github.com/apache/iceberg/pull/11419#issuecomment-2471220679 now. I think it's a bit confusing with the method naming and how `isPromotionAllowed` is called to a reader of the code. I think I'd agree with @RussellSpitzer's comment about naming this more like `ignoreTypeChange` or something along those lines. Also the comments should probably indicate that `primitive type -> narrower primitive type` is effectively allowed
--
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] Ignore schema merge updates from long -> int [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #11419:
URL: https://github.com/apache/iceberg/pull/11419#discussion_r1841671746
##########
spark/v3.5/spark/src/test/java/org/apache/iceberg/spark/source/TestDataFrameWriterV2.java:
##########
@@ -244,4 +247,132 @@ public void testMergeSchemaSparkConfiguration() throws Exception {
row(4L, "d", 140000.56F)),
sql("select * from %s order by id", tableName));
}
+
+ @TestTemplate
+ public void testMergeSchemaIgnoreCastingLongToInt() throws Exception {
+ sql(
+ "ALTER TABLE %s SET TBLPROPERTIES ('%s'='true')",
+ tableName, TableProperties.SPARK_WRITE_ACCEPT_ANY_SCHEMA);
+
+ Dataset<Row> bigintDF =
+ jsonToDF(
+ "id bigint, data string",
+ "{ \"id\": 1, \"data\": \"a\" }",
+ "{ \"id\": 2, \"data\": \"b\" }");
+
+ bigintDF.writeTo(tableName).append();
+
+ assertEquals(
+ "Should have initial rows with long column",
+ ImmutableList.of(row(1L, "a"), row(2L, "b")),
+ sql("select * from %s order by id", tableName));
+
+ Dataset<Row> intDF =
+ jsonToDF(
+ "id int, data string",
+ "{ \"id\": 3, \"data\": \"c\" }",
+ "{ \"id\": 4, \"data\": \"d\" }");
+
+ assertThatCode(() -> intDF.writeTo(tableName).option("merge-schema", "true").append())
Review Comment:
@rocco408 could you please add a comment before this line that explains why we're testing with `merge-schema=true`
--
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