morningman opened a new pull request, #69466:
URL: https://github.com/apache/doris/pull/69466
### What problem does this PR solve?
Issue Number: None
Related PR: #65329, #64304
Problem Summary:
**In short.** This restores two rules that #65329 introduced and that
branch-4.1 and branch-4.2 still follow. First, an Iceberg `MODIFY COLUMN` that
omits `NULL` / `NOT NULL` keeps the column's requiredness. Second, restating a
complex column does not turn its required nested fields, list elements or map
values optional. Master lost both rules when #64304 moved Iceberg DDL into the
connector plugin.
**Background.** #65329 (merged 07-23) made `MODIFY COLUMN` keep a column
required unless the statement says `NULL`, and made a restated STRUCT / ARRAY /
MAP keep the requiredness of everything inside it: Doris DDL cannot spell `NOT
NULL` inside `STRUCT<>`, so a nullable child there says nothing about the
user's intent. #64304 (merged 07-30) was developed in parallel and rewrote
Iceberg DDL in `fe-connector-iceberg`. The nested-path modify in the connector
still honors `nullableSpecified`, but the top-level modify and the complex type
diff were written to the pre-#65329 rules.
**The problem, and what it cost.** On master:
- `IcebergCatalogOps.modifyColumn` calls `makeColumnOptional` whenever the
new column is nullable. An omitted `NULL` is nullable by default, so `ALTER
TABLE t MODIFY COLUMN id BIGINT` turns a required `id` optional.
- `IcebergComplexTypeDiff` widens every required nested field, list element
and map value whose restated definition is nullable. That is always the case
for DDL-built types, so restating a complex column, at the top level or by
nested path, makes all of them optional.
Iceberg cannot make an optional column required again, so a statement that
only meant to widen a type removes the constraint for good, and every other
engine reading the table sees the change.
| `id INT NOT NULL`, `info STRUCT<metric INT NOT NULL, ...> NOT NULL` |
Master today | After this PR (same as branch-4.2) |
|---|---|---|
| `MODIFY COLUMN id BIGINT` | `id` becomes optional | `id` stays required |
| `MODIFY COLUMN id BIGINT NULL` | optional | optional |
| `MODIFY COLUMN info STRUCT<metric BIGINT, ...>` | `info` and every
required field inside it become optional | all stay required |
| `MODIFY COLUMN info.child STRUCT<value BIGINT>` (nested path) |
`info.child.value` becomes optional | stays required |
**How this PR fixes it.** `IcebergConnectorMetadata.modifyColumn` passes
`column.isNullableSpecified()` to `IcebergCatalogOps.modifyColumn`, as the
nested-path modify already does, and the top-level modify widens the column
only when `NULL` was written explicitly. `IcebergComplexTypeDiff` no longer
widens nested fields, list elements or map values; it still rejects narrowing
one to `NOT NULL`. A nested field becomes optional only through an explicit
`NULL` on its own path.
**Results.**
- Reproduced on a local cluster with a `file://` Iceberg table. Before:
after `MODIFY COLUMN id BIGINT` the table metadata has `id` as an optional
`long`. After: `id` is a required `long`, and `MODIFY COLUMN id BIGINT NULL`
makes it optional.
- New unit tests in `CatalogBackedIcebergCatalogOpsColumnEvolutionTest`,
adapted from branch-4.1 `IcebergMetadataOpsValidationTest`, cover the top
level, a whole STRUCT with nested STRUCT / ARRAY / MAP (with and without an
explicit `NULL`), and nested paths.
`IcebergConnectorMetadataColumnEvolutionTest` checks that the flag reaches the
catalog operations. With the old behavior restored, five of these tests fail.
- Three existing tests encoded the old widening.
`testModifyVariantColumnChangesOnlyItsMetadata` now passes an explicit `NULL`.
`testModifyStructFieldWidensNotNullToNullable` and the case-insensitive struct
test now expect the field to stay required, and are renamed accordingly.
- All 1641 tests of fe-connector-iceberg pass.
- A new block in the regression suite
`test_iceberg_nested_schema_evolution_spark_doris_interop` uses a table that
Spark creates with `id INT NOT NULL` and STRUCT columns with NOT NULL fields.
Doris runs `MODIFY COLUMN id BIGINT`, `MODIFY COLUMN widened BIGINT NULL`,
restates `info` as a whole and `info.child` by its path, and restates `payload`
with an explicit `NULL`. The block checks `DESC`, the Iceberg metadata served
by the REST catalog, and that Spark and Doris read the same rows after a Doris
insert. With the connector built from master it fails at `DESC`, where every
column and nested field is nullable. With this PR only `widened` and `payload`,
the two columns given an explicit `NULL`, become nullable, and `payload.code`
stays NOT NULL.
- The 14 other Iceberg regression suites that run `MODIFY COLUMN` against
the REST catalog pass locally with this change.
`test_iceberg_modify_binary_column` needs the one-cell `.out` fix from #69407,
which master needs anyway. `test_iceberg_on_hms_gateway_ddl_parity` needs Hive
and was not run locally; it only modifies a nullable nested field.
### Release note
Fix: `ALTER TABLE ... MODIFY COLUMN` on an Iceberg table no longer turns a
required column, or the required fields nested inside a restated complex
column, into optional ones unless the statement says `NULL` explicitly.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- [x] Unit Test
- [x] Manual test (add detailed scripts or steps below)
On a local cluster with a `file://` Iceberg catalog: `CREATE TABLE
a1_req (id INT NOT NULL, v STRING)`, `ALTER TABLE a1_req MODIFY COLUMN id
BIGINT`, then check `DESC` and the table metadata JSON (`id` stays required);
`MODIFY COLUMN id BIGINT NULL` makes it optional.
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
`MODIFY COLUMN` without `NULL` keeps an Iceberg column required, and
restating a complex column keeps its nested fields required. An explicit `NULL`
still widens the column it is written on.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
