morningman opened a new pull request, #69285:
URL: https://github.com/apache/doris/pull/69285
### What problem does this PR solve?
Issue Number: None
Related PR: #66032, #68786
Problem Summary:
**In short.** This ports to master the rule branch-4.1 got in #66032
(`IcebergMetadataOps.resolvePrimitiveTypeForModify`): when `ALTER TABLE ...
MODIFY COLUMN` restates the type an Iceberg column is read as, the Iceberg type
stays as it is. On master, every MODIFY COLUMN of an Iceberg BINARY or FIXED
column, or of such a STRUCT field, ARRAY element or MAP value, fails with
`Unsupported type for Iceberg: VARBINARY`, so their nullability, comment and
position cannot be changed at all. After this PR they can.
**Background.** Since #68786, Doris always reads an Iceberg BINARY column as
`VARBINARY` and a FIXED(n) column as `VARBINARY(n)`. MODIFY COLUMN carries the
whole column definition, so a statement that only means to change the
nullability, comment or position of such a column restates that type, for
example `MODIFY COLUMN payload VARBINARY NULL FIRST`. The nested form (`MODIFY
COLUMN s.a ...`, `arr.element`, `m.value`) works the same way. The Iceberg
connector turns the requested type into an Iceberg type before it looks at the
table (`IcebergSchemaBuilder.buildColumnType`), and then diffs it against the
current schema. `VARBINARY` has no Iceberg type there: CREATE TABLE and ADD
COLUMN reject it on purpose, as branch-4.x does, because a Doris `VARBINARY(n)`
holds up to n bytes and an Iceberg FIXED(n) exactly n.
**The problem, and what it cost.** The type build fails before the connector
ever looks at the current column, so restating the type fails too:
| Statement (current Iceberg type) | master | this PR |
|---|---|---|
| `MODIFY COLUMN payload VARBINARY NULL FIRST` (BINARY) | `Unsupported type
for Iceberg: VARBINARY` | succeeds, stays BINARY |
| `MODIFY COLUMN digest VARBINARY(16) NULL COMMENT 'digest'` (required
FIXED(16)) | same error | succeeds, stays FIXED(16) |
| `MODIFY COLUMN s.a VARBINARY COMMENT 'bytes'` (STRUCT field BINARY) | same
error | succeeds, stays BINARY |
| `MODIFY COLUMN arr.element VARBINARY(4) NULL` (required FIXED(4) element)
| same error | succeeds, stays FIXED(4) |
| `MODIFY COLUMN m.value VARBINARY NULL` (required BINARY value) | same
error | succeeds, stays BINARY |
| `MODIFY COLUMN payload VARBINARY(16)` (BINARY: not how it is read) |
`Unsupported type for Iceberg: VARBINARY` | unchanged |
So a column that users read and write as VARBINARY could not be moved,
commented, or made nullable from Doris; only a comment-only `MODIFY COLUMN ...
COMMENT '...'` worked, because it carries no type. On branch-4.1 and
branch-4.2, `IcebergMetadataOps.resolvePrimitiveTypeForModify` keeps the
current Iceberg type in the first five cases and rejects the last one (`Type
varbinary(16) is not supported for Iceberg column payload`).
**How this PR fixes it.** When the requested type does not build and it is a
`VARBINARY`, `IcebergConnectorMetadata` looks up the current type of the target
field (the same best-effort lookup it already did to rewrite the nested error
message) and keeps it if that type is read as exactly the requested
`VARBINARY`, length included (`IcebergTypeMapping.isReadAs`). The schema update
then sees an unchanged type and applies only the nullability, comment and
position the statement asks for. This is the same check the Paimon connector
already makes in `requestedColumnType`. A `VARBINARY` that restates nothing
(another length, a column or field of another type) still fails with
`Unsupported type for Iceberg: VARBINARY`, as ADD COLUMN does; any other scalar
that does not build still fails without loading the table, and a complex type
still gets the nested error message as before. A STRUCT, ARRAY or MAP type that
contains `VARBINARY` is rejected by Doris's DDL type check (`STRUCT unsupporte
d sub-type: varbinary(...)`) before it reaches the connector, on branch-4.x as
well, so it is unchanged.
**Results.**
- **Regression test**: new
`external_table_p0/iceberg/test_iceberg_modify_binary_column`. The table is
created through the REST catalog, since neither Doris nor Spark SQL can declare
a FIXED column. The suite runs the five statements above, checks with `DESC`
(comments shown) that the types are unchanged while the position and comments
changed, checks that the required column, ARRAY element and MAP value reject
NULL before and accept it after, and compares the rows with what Spark reads.
It fails at its first MODIFY with the connector before this PR and passes with
it. Seven related suites pass as well:
`test_iceberg_schema_change_complex_types`,
`test_iceberg_nested_schema_evolution_ddl`,
`test_iceberg_nested_schema_evolution_spark_doris_interop`,
`iceberg_schema_change_ddl`, `test_iceberg_varbinary`,
`test_iceberg_uuid_binary_compatibility` and
`test_iceberg_struct_schema_evolution`.
- **FE UT**: new `IcebergConnectorMetadataBinaryModifyTest` drives
`IcebergConnectorMetadata` into an in-memory Iceberg catalog. Its four
restating cases fail before this PR with `Unsupported type for Iceberg:
VARBINARY`; the fifth checks that a VARBINARY that restates nothing still fails
and leaves the schema unchanged. Turning off the length comparison, the
primitive check, the null check, or reading the current type at the top-level
column instead of the nested path each turns a case red. All 1615 tests of
fe-connector-iceberg pass.
Original author: @Gabriel39
### Release note
Iceberg: `ALTER TABLE ... MODIFY COLUMN` accepts a BINARY or FIXED column,
or such a STRUCT field, ARRAY element or MAP value, restated with the
`VARBINARY` type `DESC` shows, so its nullability, comment or position can be
changed. It used to fail with `Unsupported type for Iceberg: VARBINARY`.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [x] Regression test
- New `test_iceberg_modify_binary_column` (fails before this PR,
passes after); seven related Iceberg suites pass.
- [x] Unit Test
- New `IcebergConnectorMetadataBinaryModifyTest`; all
fe-connector-iceberg tests pass.
- [ ] Manual test (add detailed scripts or steps below)
- [ ] 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 -->
- The MODIFY COLUMN statements above succeed instead of failing.
- 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
--
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]
