AHeise commented on code in PR #29034:
URL: https://github.com/apache/flink/pull/29034#discussion_r3913380832
##########
flink-table/flink-sql-gateway/src/test/java/org/apache/flink/table/gateway/service/MaterializedTableStatementITCase.java:
##########
@@ -1151,6 +1151,38 @@ void testAlterMaterializedTableAsQueryRejectsReorder()
throws Exception {
.containsExactly("user_id", "shop_id", "ds", "order_cnt");
}
+ @Test
+ void testCreateOrAlterMaterializedTableAsQueryRejectsReorder() throws
Exception {
+ createAndVerifyCreateMaterializedTableWithData(
+ "users_shops", List.of(), Map.of(), RefreshMode.FULL);
+
+ ObjectIdentifier userShopsIdentifier =
getObjectIdentifier("users_shops");
+ ResolvedSchema oldSchema =
getTable(userShopsIdentifier).getResolvedSchema();
+ assertThat(oldSchema.getColumnNames())
+ .containsExactly("user_id", "shop_id", "ds", "order_cnt");
+
+ // CREATE OR ALTER on an existing table takes the alter path; swapping
the first two
+ // projections reorders existing columns and is rejected, matching
ALTER ... AS.
+ String materializedTableDDL =
+ "CREATE OR ALTER MATERIALIZED TABLE users_shops"
+ + " AS SELECT \n"
+ + " shop_id,\n"
+ + " user_id,\n"
+ + " ds,\n"
+ + " COUNT(order_id) AS order_cnt\n"
+ + " FROM (\n"
+ + " SELECT user_id, shop_id, order_created_at AS
ds, order_id FROM my_source"
+ + " ) AS tmp\n"
+ + " GROUP BY (user_id, shop_id, ds)";
+ OperationHandle handle = executeStatement(materializedTableDDL);
+
+ assertThatThrownBy(() -> awaitOperationTermination(service,
sessionHandle, handle))
+ .hasStackTraceContaining("reordering columns are not
supported");
+
+ // The rejected statement leaves the original schema untouched.
+
assertThat(getTable(userShopsIdentifier).getResolvedSchema()).isEqualTo(oldSchema);
+ }
Review Comment:
Done — folded all three reorder-reject ITs (bare-list CoA, ALTER ... AS, CoA
... AS) into one `@ParameterizedTest` (`rejectsReorderingExistingColumns`) over
a `reorderRejectingStatements` source; they shared the same create /
assert-reject / assert-schema-untouched skeleton.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/utils/MaterializedTableUtils.java:
##########
@@ -541,11 +412,16 @@ private static boolean typeChanged(
Column oldColumn, Column newColumn, boolean schemaDefinedInQuery) {
final DataType oldType = oldColumn.getDataType();
final DataType newType = newColumn.getDataType();
- // schemaDefinedInQuery=false: schema is inferred from the query,
which may flip
- // nullability without intent — only the base type difference is a
real change.
- return schemaDefinedInQuery
- ? !oldType.equals(newType)
- : !oldType.nullable().equals(newType.nullable());
+ if (schemaDefinedInQuery) {
+ return !oldType.equals(newType);
+ }
+ // Query-inferred nullability is a real change only when it loosens
(NOT NULL -> nullable):
+ // the stored column can no longer hold the query's possible nulls. A
tightening is
+ // tolerated.
+ final boolean baseTypeChanged =
!oldType.nullable().equals(newType.nullable());
Review Comment:
Added a bullet-point Javadoc listing what each column diff emits (add /
position / type / modify / drop) and the directional nullability handling.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlMaterializedTableNodeToOperationConverterTest.java:
##########
@@ -849,13 +849,6 @@ private static Collection<TestSpec> alterQuery() {
+ "renaming, and reordering columns are not
supported.\n"
+ "Column mismatch at position 4: Original
column is [`d` STRING], "
+ "but new column is [`d` INT]."),
- TestSpec.of(
- "ALTER MATERIALIZED TABLE base_mtbl AS SELECT a, b, c,
CAST('d' AS STRING) AS d FROM t3",
- "When modifying the query of a materialized table,
currently only support "
- + "appending columns at the end of original
schema, dropping, "
- + "renaming, and reordering columns are not
supported.\n"
- + "Column mismatch at position 4: Original
column is [`d` STRING], "
- + "but new column is [`d` STRING NOT NULL]."),
Review Comment:
Yes — tolerated now, not rejected. That case tightened nullability
(`nullable -> NOT NULL`), which the unified diff treats as a query-inference
artifact: `CAST('123' AS STRING) AS d` yields a `NOT NULL` type though the user
changed nothing about `d`'s declared type, so rejecting it would block
legitimate query edits. The stored (nullable) type is kept — no change emitted,
hence no error. The unsafe direction (`NOT NULL -> nullable`) is still
rejected; see `typeChanged` and the loosen/tighten specs in
`ValidateAndExtractColumnChangesTest`.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlNodeToOperationSqlCreateOrAlterMaterializedTableConverterTest.java:
##########
@@ -99,6 +100,36 @@ void testAlterMaterializedTableAsQueryWithDefinedSchema() {
TableChange.reset("format"));
}
+ /**
+ * Inserting a new column before more than one trailing column reorders
existing columns
+ * relative to each other. The append-only guard in {@code
validateChanges} rejects this - the
+ * gate that, in OSS, catches what the positional refresh INSERT would
otherwise miscompile.
+ */
Review Comment:
Rewritten to be plainer — a column inserted ahead of trailing columns shifts
them, which reorders existing columns, and `validateChanges` rejects that.
##########
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/operations/SqlNodeToOperationSqlCreateOrAlterMaterializedTableConverterTest.java:
##########
@@ -99,6 +100,36 @@ void testAlterMaterializedTableAsQueryWithDefinedSchema() {
TableChange.reset("format"));
}
+ /**
+ * Inserting a new column before more than one trailing column reorders
existing columns
+ * relative to each other. The append-only guard in {@code
validateChanges} rejects this - the
+ * gate that, in OSS, catches what the positional refresh INSERT would
otherwise miscompile.
+ */
+ @Test
+ void
testAlterMaterializedTableAsQueryInsertingColumnBeforeMultipleColumns() {
Review Comment:
`42 AS mid` is a physical column, not computed — every column a query
projects is physical (computed columns only come from a DDL column list, which
takes a different path that skips the position diff). Renamed to
`testAlterMaterializedTableAsQueryRejectsInsertBeforeExistingColumns` to lead
with the rejected behavior.
--
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]
