924060929 commented on PR #65329:
URL: https://github.com/apache/doris/pull/65329#issuecomment-5032954601
Focusing on the FE side only. Verified against head `6a88d6c`; not
re-raising anything already covered in the existing threads.
### 1. `legacyMode` is unreachable in production, and it silently drops an
existing behavior
`Alter.java` now always dispatches through the `ColumnPath` overload, so
`modifyTopLevelColumn(...)` is always called with `legacyMode = false`. I could
not find any production caller reaching `legacyMode = true`:
- `ExternalCatalog.modifyColumn(TableIf, Column, ColumnPosition)` has zero
callers after this PR. Note the asymmetry —
`addColumn`/`dropColumn`/`renameColumn` were rewritten to delegate to their
`ColumnPath` counterparts, but `modifyColumn` kept its old body and is now
orphaned.
- `IcebergMetadataOps` is the only `ExternalMetadataOps` impl overriding
`modifyColumn`, and it overrides the 5-arg version, so the interface bridge
never falls through to the legacy one.
- The only things keeping it alive are
`testLegacyModifyColumnTreatsNullabilityAsExplicit` and
`testLegacyComplexModifyRestoresRecursiveNullableSemantics`.
That matters because of this, in `applyStructChange` / `applyListChange` /
`applyMapChange`:
```java
if (legacyMode && !oldField.isOptional() && newField.getContainsNull()) {
updateSchema.makeColumnOptional(fieldPath);
}
```
`complexColType` has no nullability clause (`identifier COLON dataType
commentSpec?`) and `visitComplexColType` hardcodes `nullable = true`, so
`getContainsNull()` is always `true` for parsed SQL. On master the condition
therefore reduces to `!oldField.isOptional()` — `ALTER TABLE t MODIFY COLUMN s
STRUCT<...>` unconditionally promotes required nested fields to optional. After
this PR it does nothing, and nothing compensates: `applyExplicitNullableChange`
only touches the resolved target path, and `StructField` carries
`commentSpecified` but no `nullableSpecified`. If that promotion was the
statement's only intent, `UpdateSchema` ends up with an empty change set and
the statement returns success with no diagnostic.
The tests show it: `testExplicitNullableModifyMakesRequiredFieldsOptional`
asserts `makeColumnOptional` on `info` and `info.metric`, while the legacy test
on the same schema asserts `info`, `info.metric`, `info.child.value`,
`info.events.element`, `info.attrs.value`.
Suggestion: drop the `legacyMode` parameter (it only serves tests) and state
the behavior change with the migration path (`MODIFY COLUMN s.a INT NULL`) — or
reject the statement instead of silently succeeding.
### 2. `nereids/types/StructField.toSql()` backticks are unrelated to the
feature and create an inconsistency
`SqlUtils.getIdentSql()` quotes unconditionally, so every struct field name
in the nereids renderer becomes `` `id` ``. `SHOW CREATE TABLE` / `DESC` are
unaffected (they go through fe-type `catalog.StructType` / `Type`), but error
messages are: cast errors, the `arrays_overlap` / `array_union` / `array_sort`
family, `InPredicate`, aggregate-type incompatibility, nested column pruning. A
user now sees ``STRUCT<`id`:INT>`` in a cast error and `struct<id:int>` in
`DESC` for the same column.
The comment half of the change is a genuine fix — the old code emitted
`COMMENT foo` unquoted, which isn't valid SQL — please keep it. The name
quoting looks like a separate PR, or should be conditional.
Also worth knowing: `schema_change_p0/test_modify_struct.groovy` and
`test_varchar_sc_in_complex.groovy` do exact `assertEquals(x.toLowerCase(),
"struct<col:varchar(30),...>")` on DESC output. They survive only because the
catalog renderer wasn't touched.
### 3. Moving `op.validate(ctx)` after table resolution is cross-cutting
`AlterTableCommand.validate()` now resolves the table before running op
validation, so ALTER on a non-existent table reports "Unknown table" instead of
the op-level error. That's the actual reason
`test_table_level_compaction_policy.groovy` needed changing: its `CREATE TABLE`
inside `test { }` never executed (`TestAction.sql()` is a setter — only the
last statement runs), so it had been altering a table that didn't exist.
I swept `regression-test/suites` for the same pattern (CREATE + ALTER inside
one `test { }` block) and that file is the only hit, so the fix looks complete.
Still worth a line in the description, since the reordering applies to all
table types, not just Iceberg.
### 4. `toSql()` no longer emits `NULL` / `COMMENT ""`, and that string is
persisted
`AddColumnOp.toSql()` / `ModifyColumnOp.toSql()` now route through
`toSql(columnNameSql, commentSpecified)`, so both keywords are omitted when
unspecified. The round trip is semantically safe (`nullableSpecified` is only
false when `nullableType` was `DEFAULT`/`UNKNOWN`, where `isNullable` equals
what a reparse recomputes, and `validate()` never rewrites `isNullable`). But
this string goes into `AlterJobV2.rawSql` / `TableAddOrDropColumnsInfo.rawSql`
— the edit log and the CCR binlog — so the emitted SQL shape changes for
essentially every ADD/MODIFY COLUMN. Worth confirming ccr-syncer doesn't
pattern-match on it.
### Minor
- `columnDefWithPath`'s second alternative duplicates `columnDef` (minus
`DEFAULT` / `ON UPDATE`), and `visitColumnDefWithPath` duplicates ~45 lines of
`visitColumnDef`. A single rule with `(DOT colNames+=identifier)*` would
collapse both, and would let `ADD COLUMN s.b INT DEFAULT 1` produce a proper
error instead of a raw syntax error.
- `nullableSpecified` / `commentSpecified` are transient intent flags on
`Column`, a persisted catalog object, carried only to cross the
`CatalogIf.modifyColumn(TableIf, Column, ...)` boundary. Passing the
`ColumnDefinition` (or a small request object) would keep parse-time intent out
of the schema type.
- `ColumnDefinition.toSql()` and `toSql(String)` differ in comment
semantics; the javadoc on the latter doesn't say so.
- #65675 touches both `StructField` classes as well — worth deciding the
merge order.
- The checklist is empty, but this adds syntax (nested paths, `RENAME COLUMN
... TO ...`) and changes behavior (an omitted `COMMENT` no longer clears the
Iceberg doc; top-level `MODIFY COLUMN x COMMENT` on Iceberg goes from
unsupported to supported). Looks like "Behavior changed: Yes" plus a docs PR.
The syntax table also doesn't mention that `TO` is now accepted.
### Checked, not a problem
- `.map(RuleContext::getText)` on identifiers is safe — `PostProcessor`
rewrites `BACKQUOTED_IDENTIFIER` into a bare `IDENTIFIER` token during parsing,
including `` `a``b` ``.
- The new `visitQualifiedName` override can't affect other statements — all
three grammar parents are explicitly overridden, and `visitChildren` only
forwards single-child nodes.
- `validateStructTypeChanges`'s `newFields.get(i)` can't go out of range;
`ColumnType`'s "Cannot reduce struct fields" guard runs first and recurses
through ARRAY/MAP element types. A local size assertion would still make the
invariant visible at the point of use.
- The red `BE UT (macOS)` is the runner's JDK 25 vs JDK 17 issue, unrelated
to this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [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]
