Repository navigation
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
The display-only schema construction itself is sound, but the fix is incomplete across synonymous schema-reporting surfaces.
Critical checkpoints:
- Correctness:
DESCandSHOW CREATE TABLEnow preserve required top-level Iceberg fields, butSHOW [FULL] COLUMNSandinformation_schema.columnsstill report those fields as nullable. One inline issue requests consistent metadata routing. - Scan isolation: verified that display conversion allocates fresh columns and does not mutate the deliberately nullable cached/MVCC scan schema.
- Schema evolution and lifecycle: a display load uses one retained metadata generation, and explicit/Doris refresh invalidates the relevant Iceberg caches. Two broader catalog-generation concerns were checked against the base SHA and dismissed as preexisting rather than PR regressions.
- Compatibility: Iceberg views, system tables, non-Iceberg tables, mapping options, hidden/internal columns, and V3 row-lineage visibility retain their prior paths.
- Tests: the new unit and HMS regression tests cover required/optional fields, empty and populated tables, write enforcement, schema-only evolution without a new snapshot, mapping details, hidden columns, views, and scan-column isolation. They do not cover the
SHOW COLUMNS/ information-schema paths identified inline. Per the runner instruction, no builds were executed during review. At submission time, Checkstyle, license, dependency, and lightweight checks pass; FE unit tests and compile remain pending. - User focus: no additional focus points were provided.
- Review completeness: the full six-file diff converged after two rounds of normal and risk-focused review; every candidate was accepted, deduplicated, or dismissed with code/base-SHA evidence.
| && SessionVariable.enableDescribeExtendVariantColumn()) { | ||
| return new RemoteIndexSchemaProcDir(table, schema, bfColumns); | ||
| } | ||
| } else if (table instanceof IcebergExternalTable) { |
There was a problem hiding this comment.
[P1] Keep SHOW COLUMNS on the declared-nullability path
This special case fixes the proc-backed DESC, but ShowColumnsCommand still reads table.getBaseSchema() (and its WHERE form goes through information_schema.columns, whose FE producer also reads getBaseSchemaOrEmpty()). Iceberg's scan schema intentionally marks every field nullable, so a required field now reports No in DESC and NOT NULL in SHOW CREATE TABLE, yet YES in SHOW [FULL] COLUMNS / information_schema.columns. Please route those metadata-only paths through an independent display schema too, without changing scan columns, and cover both direct and filtered forms.
Use the independent Iceberg display schema for direct SHOW COLUMNS and metadata RPC output. Preserve per-table schema-load failure handling and add coverage for ordinary, FULL, LIKE, and WHERE forms.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review completed for head 95074cab9f1af39958af5da5113cf3a018381bd2.
No new blocking or substantive correctness issue was found. The current head also addresses the
existing P1 thread: direct/LIKE SHOW [FULL] COLUMNS now uses the independent Iceberg display
schema, while the WHERE form reaches the same declared-nullability data through
information_schema.columns.
Critical checkpoints:
- Correctness and isolation: display columns are freshly parsed and only top-level declared
nullability is adjusted; cached scan columns and nested scan children remain nullable. - Metadata consistency: schema, mapping-dependent types, and format-v3 hidden lineage columns are
derived from one retained Iceberg metadata generation; refresh invalidation preserves
schema-only evolution. - Compatibility and failure behavior:
DESC,SHOW CREATE TABLE, direct/LIKE/WHERE
SHOW [FULL] COLUMNS, andinformation_schema.columnsare consistent. View and non-Iceberg
fallbacks, privilege-before-load ordering, hidden-column rules, and per-table RPC offsets remain
intact. - Tests: the unit and regression additions cover empty/no-snapshot tables, required-to-optional
schema evolution, scan/write null enforcement, complex and mapped types, format-v3 hidden
columns, direct and filtered metadata forms, failure offsets, and privileges. No local build or
test was run because the review runner instructions prohibit builds. - User focus: no additional user-provided focus was supplied; the full PR was reviewed.
The expectations still encoded the old behavior where every Iceberg column was reported as nullable in DESC. Iceberg required fields now keep their declared nullability, so update the affected qt tags of iceberg_schema_change_ddl. Only the Null column changes (Yes -> No) for columns declared NOT NULL: id/name/age/score (and grade after rename). Columns created by ADD COLUMN and both partitioned tables stay nullable. Tag order is unchanged and still matches the suite's 47 qt tags.
|
run buildall |
FE Regression Coverage ReportIncrement line coverage |
…7856) ### What problem does this PR solve? Problem Summary: Iceberg columns declared `NOT NULL` are reported as nullable by `DESC`, `SHOW CREATE TABLE`, `SHOW [FULL] COLUMNS`, and `information_schema.columns`. For example, `id BIGINT NOT NULL` appears as `Null: Yes` / `IS_NULLABLE: YES` or `bigint NULL`, even though inserting NULL is rejected. Build independent display columns from one retained Iceberg metadata generation and preserve their top-level required/optional flags. These metadata-only paths use the display schema, including `SHOW COLUMNS` with `WHERE` through `information_schema.columns`. Metadata enumeration retains per-table schema-load failure handling. Scan columns keep their existing nullable semantics for historical files and schema evolution; nested-type and write-validation behavior is preserved. ### Release note Correct top-level Iceberg column nullability in `DESC`, `SHOW CREATE TABLE`, `SHOW [FULL] COLUMNS`, and `information_schema.columns`. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - [ ] 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 - Behavior changed: - [ ] No. - [x] Yes. - Does this need documentation? - [x] No. - [ ] Yes. ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
Related PR: apache#67856 Problem Summary: Iceberg columns declared NOT NULL are shown as nullable by DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema.columns. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and these commands reuse that read schema. ConnectorColumn now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. Show the declared nullability of Iceberg columns in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema.columns. - Test: Regression test, Unit Test - Behavior changed: Yes. Required Iceberg columns are now shown as NOT NULL. - Does this need documentation: No
### What problem does this PR solve? Related PR: #67856 Problem Summary: Iceberg columns declared `NOT NULL` are shown as nullable by `DESC`, `SHOW CREATE TABLE`, `SHOW [FULL] COLUMNS` and `information_schema.columns`. The connector reads every top-level column as nullable on purpose, since old data files can miss a column, and the display paths reuse that read schema. `ConnectorColumn` now also carries the nullability the source declares. The Iceberg connector sets it from the field's required flag, and these metadata commands use it. The columns used for reads and writes stay nullable. ### Release note Show the declared nullability of Iceberg columns in `DESC`, `SHOW CREATE TABLE`, `SHOW COLUMNS` and `information_schema.columns`. ### Check List (For Author) - Test - [x] Regression test - [x] Unit Test - Behavior changed: - [x] Yes. Required Iceberg columns are now shown as `NOT NULL`. - Does this need documentation? - [x] No.
…OW FULL COLUMNS (#69351) ### What problem does this PR solve? Issue Number: None Related PR: #67856, #68640 Problem Summary: **In short.** Tests only. #68640 shows required Iceberg columns as NOT NULL on master, as #67856 does on branch-4.1 and branch-4.2. The two implementations behave the same (master also covers the HTTP schema APIs and the MySQL protocol column metadata), but master's tests miss several cases that the 4.x tests check. This PR adds them; it changes no production code. Original author: @hubgeter (the 4.x tests these cases come from) **Background.** An Iceberg column declared `NOT NULL` (required) is read by Doris as nullable, so scans and expressions keep their nullable semantics, while DESC, SHOW CREATE TABLE, SHOW [FULL] COLUMNS and information_schema show the declared constraint. On master, `ConnectorColumn.sourceNullable` carries the declared nullability, and `PluginDrivenExternalTable` projects a display view next to the read schema in the same schema-cache entry. **The problem, and what it cost.** These behaviors had no test on master, so a regression in any of them would go unnoticed: | Case | 4.x test | master before this PR | |---|---|---| | A table without any snapshot shows its required columns | yes | no (the table was written before the first DESC) | | Writing NULL into a required column is still rejected | yes | no | | Relaxing a column creates no snapshot; information_schema, SHOW CREATE TABLE and later writes follow it | yes | DESC only | | SHOW FULL COLUMNS with the column comment, SHOW COLUMNS LIKE, `WHERE Null = 'YES'`, information_schema filtered by IS_NULLABLE | yes | no | | A hidden column keeps its declared nullability and is listed only in the full view | yes | no (only the visible view) | **How this PR fixes it.** - `test_iceberg_show_nullable` (REST catalog): checks the empty table (0 snapshots) before the first write; adds SHOW FULL COLUMNS, SHOW COLUMNS LIKE, `SHOW FULL COLUMNS ... WHERE Null = 'YES'` and information_schema filtered by IS_NULLABLE; checks the column comment in SHOW CREATE TABLE; writes NULL into `id` and `value` and expects "Column 'id' is declared non-nullable but contains nulls" (the message the 4.x test expects too); relaxes `value`, checks that the snapshot count stays 1 and that information_schema and SHOW CREATE TABLE follow, then writes NULL into `value` and still expects the rejection for `id`. - `PluginDrivenExternalTableTest`: a new test with a hidden required column checks that `getBaseSchemaForDisplay(full)` lists exactly the columns of `getBaseSchema(full)`, in the same order, for both values of `full`, and that the hidden column is NOT NULL only in the display view. The setup of the existing display test moves into a helper both tests use. **Results.** Both pass locally. The blocks already in `test_iceberg_show_nullable.out` are unchanged; 11 blocks are added. The new unit test fails if the display view ignores `full`, or if a copied display column loses its visibility. Co-authored-by: daidai <[email protected]>
What problem does this PR solve?
Problem Summary:
Iceberg columns declared
NOT NULLare reported as nullable byDESC,SHOW CREATE TABLE,SHOW [FULL] COLUMNS, andinformation_schema.columns. For example,id BIGINT NOT NULLappears asNull: Yes/IS_NULLABLE: YESorbigint NULL, even though inserting NULL is rejected.Build independent display columns from one retained Iceberg metadata generation and preserve their top-level required/optional flags. These metadata-only paths use the display schema, including
SHOW COLUMNSwithWHEREthroughinformation_schema.columns. Metadata enumeration retains per-table schema-load failure handling. Scan columns keep their existing nullable semantics for historical files and schema evolution; nested-type and write-validation behavior is preserved.Release note
Correct top-level Iceberg column nullability in
DESC,SHOW CREATE TABLE,SHOW [FULL] COLUMNS, andinformation_schema.columns.Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)