Repository navigation
Conversation
Member
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No blocking issues found.
Checkpoint conclusions:
- Goal and tests: The PR rejects non-null defaults for ARRAY, MAP, STRUCT, JSON, and VARIANT columns while preserving omitted defaults and explicit DEFAULT NULL. The new unit test and regression case cover create-table failures, allowed null/no-default cases, and ALTER ADD COLUMN rejection; existing expected outputs were updated for the new NULL default behavior.
- Scope and clarity: The main validation change is small and focused in ColumnDefinition, with broad test DDL updates required by the behavior change.
- Concurrency and lifecycle: Not applicable; this is DDL validation and test data update logic with no new shared mutable state, locks, or lifecycle-sensitive objects.
- Configuration and compatibility: No new configuration items. Existing persisted tables/defaults are not migrated here; the change affects new DDL validation only.
- Parallel paths: Create table and schema-change ADD COLUMN paths both flow through ColumnDefinition validation. Legacy ColumnDef translation also delegates into ColumnDefinition, so the old and Nereids paths appear covered.
- Data correctness: Rejecting string literals for complex defaults avoids storing untyped complex defaults and aligns DEFAULT() / partial-update behavior with nullable complex columns.
- Test coverage: Coverage is reasonable for the changed behavior. I did not run the test suite locally in this review runner; I reviewed the author-listed test coverage and checked the patch for whitespace issues with git diff --check.
- Observability: Not applicable; this is user-facing analysis validation with explicit error messages.
- User focus: No additional user-provided review focus was present.
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31359 ms |
Contributor
TPC-DS: Total hot run time: 169370 ms |
Member
Author
|
run cloud_p0 |
Contributor
Possible file(s) that should be tracked in LFS detected: 🚨The following file(s) exceeds the file size limit:
Consider using |
github-actions
Bot
added
the
lfs-detected!
Member
Author
|
run compile |
Contributor
FE Regression Coverage ReportIncrement line coverage |
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
May 27, 2026 04:01
bab2f97 to
a5b7348
Compare
mrhhsg
changed the title
github-actions
Bot
removed
the
lfs-detected!
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one blocking correctness issue in the new complex-default parsing path.
Critical checkpoint conclusions:
- Goal/test: The PR aims to support ARRAY/MAP/STRUCT literal defaults and adds FE unit/regression coverage for valid literals, NULL defaults, and shape-invalid literals. It does not cover type-invalid nested literal values.
- Scope/focus: The change is focused on complex default validation/parsing and default iterators.
- Concurrency/lifecycle/config/compatibility: No new concurrency, lifecycle-sensitive globals, configs, storage-format changes, or FE-BE protocol fields were found.
- Parallel paths: Nereids validation and BE default materialization were reviewed together; the identified issue is in BE nested parsing and can affect create/alter defaults once FE accepts a complex literal shape.
- Tests: Positive cases are covered, but negative cases for nested element/key/value/field type conversion are missing, which exposes the bug below.
- Observability/transactions/data writes: No new transaction or persistence logic was added; the risk is data correctness for materialized default values.
- User focus: No additional user-provided review focus was supplied.
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
May 27, 2026 06:19
a5b7348 to
dc36a4b
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
I found one correctness issue that should be fixed before approval.
Critical checkpoint conclusions:
- Goal/test coverage: The PR mostly implements literal defaults for ARRAY/MAP/STRUCT and adds FE unit plus regression coverage, but nested NULL spelling coverage is missing.
- Scope/focus: The change is focused on complex default validation/materialization.
- Concurrency/lifecycle: No new concurrency, locks, or non-trivial lifecycle management found in the touched paths.
- Config/compatibility: No config or storage-format change; metadata remains string defaults, but BE parsing must accept all FE-accepted literal forms.
- Parallel paths: FE validation and BE materialization paths were both updated, but their NULL handling is inconsistent for nested literals.
- Error handling/data correctness: One FE/BE mismatch can let valid DDL fail later during default materialization.
- Tests/results: Added outputs look deterministic, but missing a case for uppercase/mixed-case nested NULL literals.
- Observability/performance: No additional observability required; no material hot-path performance concern found.
User focus: No additional user-provided review focus was present.
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
May 27, 2026 07:19
dc36a4b to
c5101b1
Compare
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Summary
No additional blocking issues found in this pass. The current head addresses the already-raised nested strict conversion concern by unwrapping nullable nested serdes for non-NULL literals, and addresses the already-raised nested NULL casing concern with case-insensitive matching and regression coverage.
Critical checkpoint conclusions:
- Goal and tests: The PR supports literal defaults for ARRAY, MAP, and STRUCT in FE validation and BE default materialization. It includes FE unit coverage and regression coverage for create, insert, nullable defaults, alter-add-column defaults, nested NULL casing, and nested type mismatch materialization failures.
- Scope and clarity: The change is focused on complex default validation/parsing and the default iterator path.
- Concurrency and lifecycle: No new shared mutable state, locks, threads, or non-trivial lifecycle management were introduced.
- Configuration and compatibility: No new configuration items or storage/protocol format changes were introduced. Existing persisted default strings are interpreted through SerDe at materialization time.
- Parallel paths: The default iterator now receives the tablet column SerDe, covering complex defaults that could not be reconstructed from only scalar type metadata.
- Error handling: BE parsing returns non-OK Status on malformed complex defaults; the previously silent nested nullable conversion issue is fixed in the current head.
- Tests and results: New regression output is deterministic with ordered queries. I did not run tests in this review environment.
- Observability, transactions, persistence, FE-BE variables, performance: No new concerns found for these checkpoints.
User focus: No additional user-provided review focus was specified.
Member
Author
|
run buildall |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 31595 ms |
Contributor
TPC-DS: Total hot run time: 172518 ms |
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
May 27, 2026 09:59
c5101b1 to
feb5503
Compare
Member
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 29281 ms |
Contributor
TPC-DS: Total hot run time: 152532 ms |
Contributor
ClickBench: Total hot run time: 24.83 s |
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
October 9, 2026 12:37
f0b6e20 to
0196930
Compare
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Complete static review of PR #63528 at head 01969307d554e8bb57380a5381e7832867245ea9. No new inline findings. Existing P2 threads 4228442634 (signed floating zero changes on DDL replay) and 4228442626 (nested VARCHAR defaults truncate overlength input) still apply and were not reposted. Previously reported P1 issues 4226698280, 4226698283, 4226698290, 4227387142, and 4227894831 are addressed by the current DDL quoting, canonical literal validation, parser, and load mapping paths, so there are no still-applicable existing P0/P1 blockers.
Critical checkpoints:
- Goal and proof: The change adds typed literal defaults for ARRAY, MAP, and STRUCT. FE canonicalizes nested values; BE parses them in the old-rowset default iterator and direct schema change; omitted-column INSERT and load mappings use the canonical text. Added FE/BE unit tests and a regression suite exercise these paths, including errors and SHOW CREATE/LIKE replay. This invocation inspected the tests and author-provided results; it did not execute them.
- Scope and clarity: Changes stay within default validation, serialization text, materialization, load mappings, and their tests. The new parser helper is shared by DDL and explicit mapping values.
- Concurrency and lifecycle: No new threads, locks, cross-TU globals, or asynchronous ownership were introduced. The iterator holds a SerDe only when a non-NULL default needs parsing; its Field is reused during the iterator lifetime.
- Configuration and compatibility: No configuration or FE/BE protocol field was added. Canonical default text is persisted in existing column metadata and replayed through SHOW CREATE/LIKE. Old BE code cannot read newly permitted nonempty complex defaults, but the repository's documented rolling upgrade order is BE before FE; no separate supported-rollout defect was substantiated.
- Parallel paths and guards: CREATE/ALTER, light missing-column reads, direct schema change, omitted-column INSERT, and explicit/defaulted load mappings were traced. Top-level nullable SerDes are removed for explicit non-NULL materialization so nested parse failures propagate. Nested NULL is accepted according to actual child nullability. SQL MAP keys are nullable here, resolving the initial null-key suspicion.
- Tests and result data: Changed FE and BE unit tests cover canonical casts, rejection, nested NULL, three complex SerDes, and iterator behavior. Regression cases cover CREATE, ALTER, direct rewrite, load mapping, and DDL replay; their expected output matches the reviewed queries. No build, test, or result generation was performed in this review runner.
- Observability and persistence: No new distributed operation, metric, log path, EditLog record, or transaction protocol was added. Defaults use the existing column metadata and schema-change/write machinery; no new crash or atomicity path was identified in the changed code.
- Writes and performance: Default Fields are parsed at iterator setup or schema change and then inserted through established column paths. The caller skips recursive SerDe construction for NULL or absent defaults. No distinct CPU, memory-tracking, or output correctness defect remained beyond the two existing P2 threads.
- Special conditions:
NULLchecks precede quote removal, so quoted"null"remains a value; the non-NULL SerDe branch is selected only when parsing is required. The conditions follow the two materialization paths and have local explanations. - FE/BE variables: No new variable is passed between FE and BE, so there is no additional thrift send site to update.
- Transaction and data-write safety: The PR adds default values to existing column write and schema-change paths; it changes neither commit/publication order nor rowset visibility. The direct rewrite regression triggers the actual rewrite strategy with a delete predicate. No new crash recovery or concurrency boundary was introduced.
- Remaining issues: The signed-zero and nested VARCHAR P2s are already live inline threads. I found no additional substantiated bug after deduplication and final sweep.
Focus file: -light supplied no additional technical target; all 33 changed files were nevertheless reviewed. The main risk scan, normal BE pass, normal FE/test pass, separate risk-focused pass, and final changed-file sweep converged in one round with no unresolved candidate. Review status: COMPLETE. Static inspection only.
Member
Author
|
run buildall |
Member
Author
|
run compile |
Contributor
TPC-H: Total hot run time: 28906 ms |
Contributor
TPC-DS: Total hot run time: 151546 ms |
Contributor
ClickBench: Total hot run time: 24.27 s |
Member
Author
|
run check_coverage |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Issue Number: None
Related PR: None
Problem Summary: ARRAY, MAP, and STRUCT columns previously only supported DEFAULT NULL or very limited empty ARRAY defaults. This prevented valid literal defaults such as [], {}, [1, 2], {"a": 1}, or {1, "x"} from being used consistently when creating a table, adding a column, or reading default values from older rowsets. This change validates non-null complex defaults as type-appropriate literals in FE, teaches BE complex SerDes to parse these literal strings into typed Fields, and lets default iterators and direct schema change parse explicit defaults through non-nullable SerDes so type-mismatched nested values fail instead of being silently converted to NULL. Explicit nested NULL values are now accepted only when the nested SerDe is nullable, so non-nullable nested struct fields reject NULL defaults during parsing instead of reaching column insertion as a null Field. The regression coverage avoids asserting local-storage rewrite cancellation in cloud mode, where add-column schema change may finish as a metadata-only operation. INSERT fills unmentioned columns through a FE-side cast of the default literal, which is non-strict by default, so the regression exercises BE default parsing by reading rows written before a column with an invalid default was added through light schema change. JSON and VARIANT columns still only support DEFAULT NULL.
Support literal default values for ARRAY, MAP, and STRUCT columns.
- Test:
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.info.ColumnDefinitionTest
- Unit Test: ./run-be-ut.sh --run --filter=DataTypeSerDeFromStringTest.*FromFeString -j 32
- Unit Test: ./run-be-ut.sh --run --filter=DataTypeSerDeFromStringTest.structFromFeString -j 32
- Unit Test: ./run-be-ut.sh --run --filter=DataTypeSerDeFromStringTest.*:ColumnReaderTest.DefaultValue* -j 64
- Unit Test: ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.info.ColumnDefinitionTest,org.apache.doris.nereids.trees.plans.commands.AlterTableCommandTest
- Build: ./build.sh --be
- Build: ninja -C be/ut_build_ASAN src/storage/CMakeFiles/Storage.dir/schema_change/schema_change.cpp.o
- Build and Regression test: doris-local-regression --network 10.26.20.3/24 all -d datatype_p0/complex_types -s test_complex_default_value
- Format: build-support/clang-format.sh; build-support/check-format.sh; git diff --check
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d datatype_p0/complex_types -s test_complex_default_value -forceGenOut
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d datatype_p0/complex_types -s test_complex_default_value
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d schema_change_p0 -s test_dup_schema_key_add
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d schema_change_p0 -s test_unique_schema_key_change_add
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d schema_change_p0 -s test_modify_struct
- Regression test (after rebase): doris-local-regression run -d datatype_p0/complex_types -s test_complex_default_value; run -d schema_change_p0 -s test_dup_schema_key_add,test_unique_schema_key_change_add,test_modify_struct
- Regression test: doris-local-regression --network 10.26.20.3/24 run -d external_table_p0/iceberg -s test_iceberg_schema_change_complex_types
- Behavior changed: Yes. ARRAY, MAP, and STRUCT non-null literal defaults are accepted and materialized; invalid explicit defaults are rejected in both default-iterator and direct schema-change materialization paths. JSON and VARIANT non-null defaults remain rejected.
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FE validated an ARRAY/MAP/STRUCT default only by its literal shape and stored the raw SQL text, while BE parsed that text with the plain string-to-complex grammar. Values that are valid SQL literals but not plain text therefore read back wrong or failed late: `ARRAY<DATEV2> DEFAULT '[DATEV2 "2024-01-01"]'` materialized `MIN_DATE_V2`, `ARRAY<INT> DEFAULT '[1e3]'` failed when old rows were read, `ARRAY<STRING> DEFAULT '["a""b"]'` stored `a""b`, and `MAP<STRING,INT> DEFAULT '{"a":1,"a":2}'` kept both pairs although the FE literal keeps `{"a":2}`. `SHOW CREATE TABLE` also wrapped the raw default in double quotes without escaping, so `CREATE TABLE LIKE` could not replay a MAP default, and a one-argument `replace_value` load mapping on such a column hit the scalar-only precondition of `ColumnDef.validateDefaultValue`.
FE now parses the default as a SQL literal, casts every nested value to the declared nested type with strict checking, and stores a canonical text (unquoted numbers, bool as 1/0, double quoted strings/dates/ips, nested brackets, NULL). Type mismatches, typed or exponent literals and repeated map keys are therefore resolved at DDL time, and BE only ever parses the canonical form through the default value iterator, schema change, and the INSERT cast. Nested string values that contain quotes or backslashes are rejected, because neither BE parser decodes escapes and the DDL parser keeps the default text verbatim. `Column.toSql()` wraps a default that contains double quotes in single quotes so SHOW CREATE TABLE output replays through CREATE TABLE LIKE, and load `default_value`/`replace_value` mappings validate complex columns through the same canonicalizer.
### Release note
None
### Check List (For Author)
- Test: Regression test / Unit Test
- ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.info.ColumnDefinitionTest,org.apache.doris.catalog.ColumnTest
- ./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.AlterTableCommandTest
- ./run-be-ut.sh --run --filter='DataTypeSerDeFromStringTest.*'
- doris-local-regression run -d datatype_p0/complex_types -s test_complex_default_value
- doris-local-regression run -d schema_change_p0 -s test_dup_schema_key_add,test_unique_schema_key_change_add,test_modify_struct
- Behavior changed: Yes. Complex column defaults are type checked at DDL time and stored in canonical form; nested string defaults may not contain quotes or backslashes.
- Does this need documentation: No
…ng map keys
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Follow-up fixes for literal defaults of ARRAY/MAP/STRUCT columns.
1. `default_value('[1e3]')` and `replace_value(null, '[1e3]')` mappings on an
ARRAY<INT> column passed FE validation (which canonicalizes the text to
`[1000]`) but the original literal was still sent to BE, whose string cast
writes a NULL element in non-strict load and fails in strict load.
`NereidsLoadScanProvider.transformHadoopFunctionExpr` now replaces the
validated argument with the canonical text.
2. `MAP<INT, INT> DEFAULT '{"01": 1, "1": 2}'` rendered two entries with key
`1`; BE materialized both for old rows while replaying the text as a SQL
literal kept only the last one. Keys that collide after the cast to the key
type are rejected at DDL time.
3. `Segment::new_default_iterator` built a (recursive) SerDe for every missing
column of every old segment even when the column has no default or
`DEFAULT NULL`, which never parses anything. The SerDe is now built only for
a non-NULL default text and `DefaultValueColumnIterator` asserts it exists
when it must parse; the iterator's unused FieldType/precision/scale/len
fields are removed.
4. The regression case labelled as direct schema change actually took BE's
linked strategy; a delete predicate is now added before the ALTER so BE
selects `sc_directly` and materializes the complex defaults.
None
- Test: Unit Test / Regression test
- FE UT: ColumnDefinitionTest, NereidsLoadScanProviderTest
- BE UT: ColumnReaderTest, DataTypeSerDeFromStringTest
- Regression: datatype_p0/complex_types/test_complex_default_value,
schema_change_p0 (test_dup_schema_key_add, test_modify_struct,
test_unique_schema_key_change_add)
- Behavior changed: Yes (map defaults whose keys collide after the cast are rejected; explicit complex load mapping values are canonicalized)
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Follow-up fix for literal defaults of ARRAY/MAP/STRUCT columns.
`ComplexTypeDefaultValue.canonicalize` parsed the default text with the
regular SQL literal builder, which first casts the items of an array literal
and the keys and values of a map literal to a common type inferred from the
items. With the default `enable_strict_cast=false` an item that cannot be
cast is silently replaced by NULL, so `ARRAY<DATEV2> DEFAULT
'[DATEV2 "2024-01-01", 1]'` was accepted and stored as `["2024-01-01", NULL]`:
old rows and INSERTs that omit the column got a NULL the user never wrote.
The default text is now parsed with a builder that keeps every array item and
map key/value exactly as written; `render` casts each of them to the declared
nested type with `checkedCastTo`, which rejects the invalid item at DDL time
regardless of the session setting. `NereidsParser.parseExpression` gains an
overload taking the builder, mirroring `parseSingle`.
### Release note
None
### Check List (For Author)
- Test: Unit Test / Regression test
- FE UT: ColumnDefinitionTest, NereidsLoadScanProviderTest, AlterTableCommandTest
- Regression: datatype_p0/complex_types/test_complex_default_value
- Behavior changed: Yes (a nested default value that the SQL parser would have coerced to NULL is now rejected)
- Does this need documentation: No
…n test ### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Rebasing onto master picked up apache#65684, which deleted `DefaultValueColumnIterator` and moved default-value parsing into `Segment::get_default_value_field`, together with a new `constant_column_iterator_test.cpp`. That file's `NonEmptyComplexTextDefaultsAreRejected` asserted the pre-PR restriction that a non-empty ARRAY/MAP/STRUCT literal default is rejected (`NOT_IMPLEMENTED_ERROR`), which no longer holds once this branch's earlier commits teach the complex SerDes to parse such literals. Replace it with `NonEmptyComplexTextDefaultsAreParsed`, asserting that `[1]`, `{"k":7}`, and a positional struct literal are parsed into the matching typed `Field` through `Segment::get_default_value_field`. ### Release note None ### Check List (For Author) - Test: Unit Test - `./run-be-ut.sh --run --filter="ConstantColumnIteratorTest.*" -j 32` (34 tests, all pass) - Behavior changed: No (test-only; the behavior change itself is in the earlier commits) - Does this need documentation: No
mrhhsg
force-pushed
the
fix/reject-complex-type-default
branch
from
October 13, 2026 10:15
0196930 to
5629205
Compare
Member
Author
|
/review-light |
Contributor
There was a problem hiding this comment.
Reviewed pinned head 56292050219133897db92ad055709554f3e16736 against the supplied authoritative diff, covering all 30 changed files and their relevant callers/consumers. Request changes for two new P1 inline findings; the new P2 inline finding should also be addressed.
One P1 affects light schema change: selecting one field of a nonempty defaulted STRUCT on an old rowset inserts the full default into a pruned destination and fails. The other P1 affects MoW partial-update new keys: a nested STRING NULL default is persisted as the string "NULL". The P2 concerns recursive default buffers and retained iterator copies that bypass memory accounting. For the supplied -light focus, light-schema old-row reads, post-ALTER INSERT and the direct-rewrite counterpart were specifically traced.
Existing P1 concerns about permissive nested-item coercion, load mapping canonicalization, typed literals, quoted strings and DDL replay are fixed at this head. No previously reported P0/P1 was independently confirmed to remain, so existing_blocking_comment_ids is empty. Existing P2 nested VARCHAR truncation and negative-zero replay still apply and were not reposted.
Critical checkpoint conclusions:
- Goal and proof: Typed ARRAY/MAP/STRUCT literal defaults are implemented, but pruned old-row queries fail and partial updates can persist an incorrect nested value. FE/BE unit and regression coverage exists; no tests were executed in this static review.
- Scope/reuse: Changes are focused on defaults and reuse the parser, scalar SerDes and schema-change mapping.
- Concurrency/locking: No new threads, shared mutable state or locks; existing storage/schema-change lock and visibility ordering is unchanged.
- Lifecycle: Fields own their nested data and request/job lifetimes are coherent; no new static-initialization or COW issue. Their owned-memory accounting is the P2 finding.
- Configuration: No new configuration; nested pruning is already enabled by default.
- Compatibility: No wire/block layout change; existing NULL/empty ARRAY handling remains. The new feature requires upgraded BEs; no supported rolling-upgrade regression was substantiated.
- Thrift/protobuf: No definitions changed; field-ID checks are inapplicable.
- Parallel paths: CREATE/ADD/MODIFY, INSERT/DEFAULT, load replacements/default mappings, old-row readers, schema-change strategies, cloud reuse, partial updates, the row-store JSON fallback and connector restrictions were traced. Pruned scan defaults and partial-update JSON default parsing are the missed parallel consumers.
- Conditions/errors: Invalid shapes, casts, key collisions and nonnullable nested NULL are rejected; Status/analysis errors propagate. Empty STRUCT child defaults agree across ordinary consumers.
- Coverage: Tests cover normal literals, invalid input, replay, light old-row reads, a delete-predicate-triggered direct rewrite and load mappings. They miss nonempty pruned STRUCT/container reads, partial-update new keys with nested STRING NULL defaults, and large-default accounting; empty STRUCT ADD/rewrite, sorted and cloud/MoW execution are not demonstrated.
- Result files: New goldens and load CSVs were inspected and are consistent with the covered examples; generation provenance and runtime success were not verified.
- Observability: Existing parse/type diagnostics and schema-change logs suffice for these paths; the P2 buffers are absent from tracker observability.
- Persistence/failover: Canonical text uses existing Column/TabletColumn metadata. No EditLog or failover protocol change was found.
- Writes/atomicity: Existing rewrite/publication/version and MoW sequencing remains; parsing errors propagate before rewrite publication. No new crash-lifetime issue was identified, but partial-update default fills can persist wrong values as reported inline.
- FE-BE variables: Existing default text and nested type transport is reused; no new variable was added.
- Performance: Parsing is linear, and NULL/absent defaults avoid SerDe construction. Expanded Field storage/copies require the memory-accounting fix.
- Other concerns: No further distinct verified issue beyond the three new inline findings and the two existing P2 threads.
Review limits: static inspection only; no builds, product tests, source edits or executed reproductions. Author-reported validation and supplied goldens are not test execution by this review. Convergence is recorded separately by the submission helper.
Converged after 3 review rounds: all three latest-round reviewers returned NO_NEW_VALUABLE_FINDINGS, required static coverage is complete, and every candidate/risk is accepted, deduplicated or dismissed with evidence. No unresolved review candidate remains.
Review converged after 3 round(s).
…mplex defaults ### What problem does this PR solve? Issue Number: None Related PR: apache#63528 Problem Summary: Follow-up fixes for literal defaults of ARRAY/MAP/STRUCT columns. 1. Nested column pruning reads only the accessed STRUCT fields, so after `ADD COLUMN s STRUCT<a:INT,b:STRING> DEFAULT '{7, "x"}'` on a table with old rows, `SELECT struct_element(s, 'b')` created a STRUCT<b:STRING> destination while the old segment's constant default was parsed with the declared type and failed with `Cannot insert value of different size into struct`; STRUCT items of an ARRAY and STRUCT values of a MAP failed the same way. `Segment::new_column_iterator` now receives the materialized read type (`ReadSchema::data_type`) and, when access paths exist for the column, projects a `ConstantColumnIterator`'s Field onto it (`ConstantColumnIterator::project_to_read_type`): STRUCT fields are selected by the read type's names, ARRAY items and MAP values recurse, keys, scalars, NULL and the `{}` default are kept. 2. A partial update of a new key fills omitted columns through the BE JSON parser (`default_from_string`), whose nullable SerDe only recognizes a lower case `null`, so the canonical `[NULL]` text was stored as `["NULL"]`. The canonical text now renders a nested NULL as `null`, which every BE consumer of the text accepts. 3. A nested VARCHAR(n)/CHAR(n) value longer than n was truncated by the cast to VARCHAR(n); it is now rejected with `Default value is too long`, like the scalar default check. 4. `ARRAY<DOUBLE> DEFAULT '["-0"]'` stored `[-0.0]`, but replaying the SHOW CREATE TABLE text parsed `-0.0` through BigDecimal and stored `[0.0]`. A SQL numeric literal has no negative zero, so a float zero is always rendered as `0.0`. ### Release note None ### Check List (For Author) - Test: Unit Test / Regression test - BE UT: `ConstantColumnIteratorTest.*` (new `ComplexDefaultIsProjectedToPrunedReadType`), `*SegmentIteratorExprZonemap*`, `*IndexBuilder*` - FE UT: `ColumnDefinitionTest` - Regression: `datatype_p0/complex_types/test_complex_default_value` (pruned reads of light-added defaults, MoW partial update of a new key, VARCHAR/CHAR length rejects, negative zero replay) - Behavior changed: Yes (nested NULL in a complex default is stored as `null`; overlength nested VARCHAR/CHAR defaults are rejected; a float `-0.0` default is stored as `0.0`) - Does this need documentation: No
Member
Author
|
/review-light |
1 similar comment
Member
Author
|
/review-light |
Member
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 38 changed files at 0d9f0c992a87a85708cbfd7a25baf8de6aba0b8c by static inspection. One new P2 finding remains: generated light ALTER SQL escapes canonical nested quotes, so CCR replay rejects otherwise valid complex defaults. The inline comment identifies the separate ALTER serializer/consumer; the prior SHOW CREATE/LIKE issue is fixed.
The existing P1 reports for SQL-literal coercion, nested escapes/types/NULLs, explicit load mappings, DDL quoting and pruned old-row defaults are addressed at this head; no existing P0/P1 blocking IDs remain. Existing P2 thread 4257770328 about untracked recursive Field buffers still applies and was deduplicated.
Critical checkpoint conclusions:
- Goal and proof: ARRAY/MAP/STRUCT literal defaults now have target-aware canonicalization and BE parsing; test sources cover the principal materializers. ALTER binlog replay remains incomplete as described inline.
- Scope and clarity: changes are focused on defaults, required iterator type propagation, and related tests.
- Concurrency and locks: parsing is local and projection changes scan-owned values; existing cache initialization, schema-change locking and lock order are preserved.
- Lifecycle and static initialization: recursive type owners outlive projection; no new ownership cycle, worker lifetime or cross-TU initializer dependency found.
- Configuration: no configuration added.
- Compatibility: existing NULL/empty/scalar defaults and metadata layouts are preserved; new literals require feature-capable BE. The generated ALTER replay failure is the verified compatibility issue.
- Thrift/protobuf: no definition or field-ID changes.
- Parallel paths: ordinary INSERT, explicit/fallback load mappings, missing-segment scans, direct schema change, partial updates, pruned reads and rowid fetch were traced; the distinct ALTER text path has the inline finding.
- Conditions: shape/type/nullability, ambiguous quoting, key collisions, length and finite-number checks address concrete inputs; recursive projection assumptions were checked against FE pruning.
- Test coverage: FE/BE unit sources and CREATE/LIKE, light/direct ALTER, load mapping and partial-update regressions were reviewed. Serialized ALTER replay coverage is missing.
- Test results: new expected values were reconciled with SQL/canonical forms; execution and golden-generation provenance were not independently verified.
- Observability: parse failures reach existing Status/AnalysisException boundaries; no new distributed lifecycle needs additional metrics or INFO logging.
- Persistence and failover: existing catalog/tablet default fields and metadata replay are reused; exported light-schema rawSql has the reported replay defect.
- Writes and atomicity: default materialization remains before existing commit boundaries; visible-version, rowset publication and MoW delete-bitmap phases are unchanged.
- FE-BE values: canonical text travels through existing default/type metadata; no new wire variable introduced.
- Performance and memory: recursive work follows literal size; the previously reported untracked Field allocations remain applicable, without a duplicate inline comment.
- Other issues: all initial risks and candidate dispositions have source evidence; no additional verified issue remains outside the inline set and known thread.
For the supplied -light focus, old-row default materialization and nested projection were inspected, and the new finding specifically covers light ADD COLUMN binlog replay. Validation was static only: no builds, tests, running cluster, mixed-version deployment or source edits were performed.
Review converged after 2 rounds: all three latest reviewers returned NO_NEW_VALUABLE_FINDINGS, all changed-file coverage is complete, and all candidate/risk dispositions are recorded. No unresolved static coverage question remains.
Review converged after 2 round(s).
| throw new AnalysisException("Struct type column default value just support null"); | ||
| if (type.isArrayType() || type.isMapType() || type.isStructType()) { | ||
| defaultValue = Optional.of(new DefaultValue( | ||
| ComplexTypeDefaultValue.canonicalize(type, defaultValue.get().getValue()))); |
Contributor
There was a problem hiding this comment.
[P2] Preserve complex defaults in ALTER binlog SQL
With light schema change, ALTER TABLE t ADD COLUMN m MAP<STRING,INT> DEFAULT '{"a":1}' now succeeds, but ColumnDefinition.toSql() still applies SqlUtils.escapeQuota and emits DEFAULT "{\"a\":1}". Alter.processAlterOlapTable passes this text into TableAddOrDropColumnsInfo.rawSql, and CCR re-executes it. The DDL parser only removes the outer quotes, so replay passes {\"a\":1} to the canonicalizer and rejects it, stopping synchronization. Apply the replay-safe quoting used by Column.toSql() to this ALTER serializer and cover a serialized ADD COLUMN round trip. This is a separate consumer from the already-fixed SHOW CREATE/LIKE thread.
Contributor
There was a problem hiding this comment.
Reviewed pinned head 0d9f0c992a87a85708cbfd7a25baf8de6aba0b8c. No new verified findings. All 38 changed files and related consumers were reviewed; the FE, BE and independent risk reviewers each returned NO_NEW_VALUABLE_FINDINGS. Review converged in 1 round, with no unresolved candidate or uncovered changed-file scope.
Two previously reported P2 concerns still apply and are not reposted: ALTER binlog SQL quoting still uses the escaping serializer in ColumnDefinition.toSql, and parsed complex-default memory accounting still retains untracked recursive Field buffers/copies. The seven existing explicitly P1 reports were independently rechecked: SHOW CREATE/LIKE quoting and unsafe strings, typed literal conversion, explicit load mappings, inferred-item coercion, pruned default projection and partial-update nested NULL now have their concrete fixes. No existing P0/P1 finding was confirmed to remain applicable, so the blocking-ID set is empty.
The reviewed value flow is:
flowchart LR
D[CREATE / ALTER / mapping validation] --> C[Strict FE canonical literal]
C --> M[Existing Column default metadata]
M --> I[INSERT / load / MERGE string cast]
M --> F[Old-segment Field parsing and projection]
M --> S[Direct schema-change Field backfill]
M --> J[Partial-update and row-store JSON defaults]
Critical checkpoint conclusions:
| Checkpoint | Conclusion |
|---|---|
| Goal and proof | Literal ARRAY/MAP/STRUCT defaults are canonicalized and recursively materialized across the inspected flows. FE/BE unit and regression source covers the principal positive/negative cases; execution was not performed. |
| Scope and reuse | Changes are focused on defaults, reused canonicalization/strict-conversion helpers, necessary iterator type propagation and related tests. |
| Concurrency and locks | Parsing state is local; cached default Fields are copied before iterator-local projection. No new thread, shared mutation, lock acquisition or lock-order change found. |
| Lifecycle and initialization | Fields/iterators have owning lifetimes; no new ownership cycle or cross-TU static initialization dependency. Memory accounting remains the known P2 above. |
| Configuration | No configuration added or dynamic-setting propagation introduced. |
| Compatibility | Existing default metadata and block/wire/storage layouts are reused. Newly enabled nonempty defaults need supporting BE binaries; mixed-version behavior was not executed. |
| Thrift/protobuf IDs | No files under gensrc/thrift or gensrc/proto changed. |
| Parallel paths | CREATE/ALTER, SHOW CREATE/LIKE, mappings, INSERT/DEFAULT, MERGE, old-segment scans, direct backfill, partial updates and row-store fallback traced. Statistics, row-id and normal iterator callers pass materialized read types. The existing ALTER SQL concern remains. |
| Conditions and errors | Shape/type/arity/key-collision/length/finite-value checks have concrete failure scenarios. Non-NULL nested defaults use strict parsing; explicit NULL requires a nullable child. New Status results propagate; declared/read projection assertions match upstream shapes. |
| Test coverage | Source covers nullable/non-nullable cases, nesting, empty containers, malformed values, casts, old/new rows, field pruning, loads, LIKE and partial updates. The delete predicate forces direct rewrite in the local-mode test; cloud execution was not verified. |
| Expected results | All 11 ordered query labels match the new golden sections, row widths and inspected literal/mapping values agree, and changed error expectations match FE validation. Golden generation provenance was not independently established. |
| Observability | Existing parse/validation errors and schema-change diagnostics cover these paths; no new background workflow needing additional metrics identified. |
| Persistence and failover | Canonical text uses existing Column/TabletColumn fields and schema maps. Light-schema-change EditLog writes and catalog replay preserve those maps; no new journal object or transaction transition. |
| Writes, atomicity and crash behavior | Existing rowset/schema-change commit and publish boundaries remain. No new transaction, visible-version or delete-bitmap ordering change; no distinct new crash/leak/deadlock mechanism substantiated. |
| FE-BE variables | No new transmitted variable. Existing default text is reused, and the new read_type argument is internal BE API plumbing at every inventoried caller. |
| Performance and memory | Parsing/projection scales with literal size during planning/iterator setup, with no new per-row parser. Recursive allocation/copy tracking remains the previously reported P2; no distinct additional performance finding. |
| Other risks | Root STRUCT reorder/drop, stale positional defaults after field append and isolated size/cardinality/IS NULL pruning were rechecked: existing name/order/arity/default-equality and type-preservation guards block the suspected failures. |
For the supplied -light focus, light schema change and old-row default materialization received particular attention, including recursive pruned STRUCTs, metadata-only access paths and retained defaults during later schema changes. No additional issue was found beyond the two known P2 reports.
This was a static-only review: no builds, test runs or source edits. Inspected unit tests, regression goldens and author-reported validation are not executed evidence from this invocation. Runtime, external-service/cloud and rolling-upgrade behavior remain validation limits; no concrete unresolved review question remains.
Review converged after 1 round(s).
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
2 participants
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: None
Problem Summary: ARRAY, MAP, and STRUCT columns previously only supported DEFAULT NULL or very limited empty ARRAY defaults. This prevented valid literal defaults such as
[],{},[1, 2],{"a": 1}, or{1, "x"}from being used consistently when creating a table, adding a column, or reading default values from older rowsets. This change validates non-null complex defaults as type-appropriate literals in FE, teaches BE complex SerDes to parse these literal strings into typed Fields, and lets default iterators and direct schema change parse explicit defaults through non-nullable SerDes so type-mismatched nested values fail instead of being silently converted to NULL. Explicit nested NULL values are now accepted only when the nested SerDe is nullable, so non-nullable nested struct fields reject NULL defaults during parsing instead of reaching column insertion as a null Field. The regression coverage avoids asserting local-storage rewrite cancellation in cloud mode, where add-column schema change may finish as a metadata-only operation. INSERT fills unmentioned columns through a FE-side cast of the default literal, which is non-strict by default, so the regression exercises BE default parsing by reading rows written before a column with an invalid default was added through light schema change. JSON and VARIANT columns still only support DEFAULT NULL.FE stores the default in a canonical text rather than the raw SQL spelling: the value is parsed as a SQL literal, every nested value is cast to the declared nested type with strict checking, and the result is rendered with unquoted numbers (booleans as
1/0), double quoted strings/dates/ips, nested brackets andNULL. Type mismatches, typed or exponent literals such as[DATEV2 "2024-01-01"]or[1e3], and repeated map keys are therefore resolved at DDL time, and BE parses the same canonical text through the default value iterator, schema change, and the INSERT cast. Nested string values that contain quotes or backslashes are rejected because neither BE parser decodes escapes and the DDL parser keeps default text verbatim when SHOW CREATE TABLE output is replayed.SHOW CREATE TABLEwraps a default that contains double quotes in single quotes soCREATE TABLE LIKEcan replay it, and loaddefault_value/replace_valuemappings validate complex columns through the same canonicalizer.Release note
Support literal default values for ARRAY, MAP, and STRUCT columns.
Check List (For Author)
./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.info.ColumnDefinitionTest,org.apache.doris.catalog.ColumnTest./run-fe-ut.sh --run org.apache.doris.nereids.trees.plans.commands.AlterTableCommandTest./run-be-ut.sh --run --filter='DataTypeSerDeFromStringTest.*' -j 32./run-be-ut.sh --run --filter=DataTypeSerDeFromStringTest.*:ColumnReaderTest.DefaultValue* -j 64./build.sh --be --febuild-support/clang-format.sh;build-support/check-format.sh;git diff --checkdoris-local-regression --network 10.26.20.3/24 run -d datatype_p0/complex_types -s test_complex_default_value(-genOutfirst, then compare mode)doris-local-regression --network 10.26.20.3/24 run -d schema_change_p0 -s test_dup_schema_key_add,test_unique_schema_key_change_add,test_modify_structdoris-local-regression --network 10.26.20.3/24 run -d external_table_p0/iceberg -s test_iceberg_schema_change_complex_types