mxm commented on code in PR #18357:
URL: https://github.com/apache/iceberg/pull/18357#discussion_r4376141350
##########
flink/v2.3/flink/src/test/java/org/apache/iceberg/flink/sink/dynamic/TestCompareSchemasVisitor.java:
##########
@@ -33,6 +33,57 @@
class TestCompareSchemasVisitor {
+ @Test
+ void testEmptyStructSame() {
+ // Empty structs (for example an Avro zero-field marker record) must
compare SAME
+ // against the identical table schema instead of requesting a schema
update.
+ assertThat(
+ CompareSchemasVisitor.visit(
+ new Schema(
+ required(1, "id", StringType.get()),
+ optional(2, "marker", StructType.of())),
+ new Schema(
+ required(1, "id", StringType.get()),
+ optional(2, "marker", StructType.of())),
+ CASE_SENSITIVE,
+ PRESERVE_COLUMNS))
+ .isEqualTo(CompareSchemasVisitor.Result.SAME);
+ }
+
+ @Test
+ void testNestedEmptyStructSame() {
+ assertThat(
+ CompareSchemasVisitor.visit(
+ new Schema(
+ optional(
+ 1,
+ "outer",
+ StructType.of(optional(2, "marker",
StructType.of())))),
+ new Schema(
+ optional(
+ 1,
+ "outer",
+ StructType.of(optional(2, "marker",
StructType.of())))),
+ CASE_SENSITIVE,
+ PRESERVE_COLUMNS))
+ .isEqualTo(CompareSchemasVisitor.Result.SAME);
+ }
+
+ @Test
+ void testEmptyStructAgainstNonEmptyTableStruct() {
+ // An empty input struct against a table struct with an extra optional
field still
+ // needs a data conversion; the empty-struct case must not mask real
differences.
+ assertThat(
+ CompareSchemasVisitor.visit(
+ new Schema(optional(1, "marker", StructType.of())),
+ new Schema(
+ optional(
+ 1, "marker", StructType.of(optional(2, "extra",
StringType.get())))),
+ CASE_SENSITIVE,
+ PRESERVE_COLUMNS))
+ .isEqualTo(CompareSchemasVisitor.Result.DATA_CONVERSION_NEEDED);
+ }
+
private static final boolean CASE_SENSITIVE = true;
private static final boolean CASE_INSENSITIVE = false;
Review Comment:
Could we move the new tests below the constants?
##########
flink/v1.20/flink/src/test/java/org/apache/iceberg/flink/sink/dynamic/TestCompareSchemasVisitor.java:
##########
@@ -33,6 +33,57 @@
class TestCompareSchemasVisitor {
Review Comment:
Should we add a test to TableUpdater 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
