Repository navigation
[test](iceberg) Cover required columns on empty tables, writes and SHOW FULL COLUMNS - #69351
Merged
morningman merged 1 commit intoOct 21, 2026
Conversation
…OW FULL COLUMNS ### What problem does this PR solve? Issue Number: None Related PR: apache#67856, apache#68640 Problem Summary: apache#68640 shows required Iceberg columns as NOT NULL in DESC, SHOW CREATE TABLE, SHOW COLUMNS and information_schema on master, as apache#67856 does on branch-4.1 and branch-4.2. The implementation is equivalent and covers more statements, but master's tests miss cases that the 4.x tests check: - a table without any snapshot already shows its required columns as NOT NULL; - only the metadata view is NOT NULL: writing NULL into a required column is still rejected ("Column 'id' is declared non-nullable but contains nulls"), and once a column is relaxed it accepts NULL while id still rejects it; - relaxing a column creates no snapshot, and information_schema and SHOW CREATE TABLE follow it; - SHOW FULL COLUMNS with the column comment, SHOW COLUMNS LIKE, SHOW FULL COLUMNS ... WHERE `Null` = 'YES', and information_schema filtered by IS_NULLABLE; - a hidden column keeps its declared nullability and is listed only in the full display view. This adds them to test_iceberg_show_nullable and PluginDrivenExternalTableTest. No production code changes. ### Release note None ### Check List (For Author) - Test: Regression test / Unit Test - Regression test: test_iceberg_show_nullable against the iceberg docker REST catalog; the blocks already in its .out are unchanged - Unit Test: PluginDrivenExternalTableTest, which fails if the display view ignores `full` or a copied display column loses its visibility - Behavior changed: No - Does this need documentation: No Co-authored-by: daidai <[email protected]> Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No actionable defects found in all three changed files at head df697f2. The test-only change adds meaningful coverage for Iceberg's declared nullability while preserving nullable read semantics.
Critical checkpoint conclusions:
- Goal and proof: the unit test exercises real schema conversion/display projection for visible and hidden columns; the regression checks empty-table metadata, required writes, and relaxed constraints. Static inspection supports the intended coverage.
- Scope and reuse: changes stay within the existing unit test, regression suite, and expected output. The shared setup helper preserves every prior display-schema assertion.
- Concurrency and locking: no threads, shared production state, lock ordering, or blocking behavior changes; test statements run sequentially.
- Lifecycle and initialization: mocks/cache entries are local to each unit test, ConnectContext cleanup remains in place, and regression setup drops its catalog/database before reuse while retaining final state for debugging.
- Configuration: no configuration additions or dynamic-setting changes.
- Compatibility: no production API, storage format, plugin SPI, or rolling-upgrade changes.
- Thrift/protobuf: no definition changes; cross-branch field-ID checks are inapplicable.
- Parallel paths: visible/full display schemas, direct SHOW/FULL/LIKE, rewritten WHERE, information_schema, and SHOW CREATE are covered. Read-schema nullability remains independently asserted.
- Conditions: the existing external-environment gate and the full/visible test loop are appropriate; no new production conditional behavior.
- Test coverage and errors: the new cases check id and value rejection independently, successful NULL after relaxing value, and continued id rejection. Expected errors use the test/exception DSL and agree with the pinned Arrow 24 required-field check.
- Expected results: all 17 SQL tags match the output blocks and column counts. Added values agree with the producing code, including different direct-SHOW versus information_schema type/collation representations. Rowset queries are sorted; schema order and scalar counts are stable.
- Observability: existing assertion messages, exception checks, and golden comparisons identify failures; no new runtime path warrants logs or metrics.
- Transactions and persistence: the snapshot assertions agree with one initial append and schema-only ALTER; failed writes do not reach commit. No journal, replay, or failover implementation changes.
- Data writes and atomicity: added writes are controlled test data; subsequent snapshot/data checks cover accidental publication and retained committed rows. Crash handling itself is unchanged.
- FE/BE passing: no transmitted variables or protocol changes. The existing Iceberg schema reaches Arrow with source-required fields while FE reads remain nullable.
- Performance and memory: bounded test inputs and metadata queries add no production hot-path allocations or complexity; the existing one-cache-read assertion remains intact.
- Other concerns and user focus: no additional user focus was supplied. No existing inline threads or confirmed P0/P1 blockers exist. All three initial risk focuses were dismissed with implementation evidence.
Both assigned reviewers returned NO_NEW_VALUABLE_FINDINGS on the same empty finding set in round 1, and the main agent independently verified their conclusions. Review converged; no unresolved candidate or uncovered changed path remains.
Validation limit: this was a static-only review. No builds, tests, cluster execution, source edits, or output regeneration were performed. Live REST/MinIO behavior and the expected-output file's generation provenance were not independently established.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28502 ms |
Contributor
TPC-DS: Total hot run time: 150287 ms |
Contributor
ClickBench: Total hot run time: 24.22 s |
Contributor
FE UT Coverage ReportIncrement line coverage `` 🎉 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
924060929
approved these changes
Oct 21, 2026
924060929
left a comment
924060929
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the FE test changes at df697f2. The shared fixture preserves the existing display/read-schema assertions, and the new test covers hidden required columns in both full and visible views. Regression inputs and expected output cover empty tables, SHOW filtering, NULL rejection and constraint relaxation. No blocking issue found. Source review only; no local test execution.
924060929
approved these changes
Oct 21, 2026
Contributor
FE Regression Coverage ReportIncrement line coverage |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.sourceNullablecarries the declared nullability, andPluginDrivenExternalTableprojects 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:
WHERE Null = 'YES', information_schema filtered by IS_NULLABLEHow 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 intoidandvalueand expects "Column 'id' is declared non-nullable but contains nulls" (the message the 4.x test expects too); relaxesvalue, checks that the snapshot count stays 1 and that information_schema and SHOW CREATE TABLE follow, then writes NULL intovalueand still expects the rejection forid.PluginDrivenExternalTableTest: a new test with a hidden required column checks thatgetBaseSchemaForDisplay(full)lists exactly the columns ofgetBaseSchema(full), in the same order, for both values offull, 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.outare unchanged; 11 blocks are added. The new unit test fails if the display view ignoresfull, or if a copied display column loses its visibility.Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)