Repository navigation
[fix](fe) Resolve DEFAULT(column) on Iceberg tables from the write schema - #69488
morningman wants to merge 4 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed exact head bda3e28cdab081693dc2dd8d739d784005b7ce9d against the supplied authoritative aggregate patch. Five new verified findings: one P1 and four P2, all inline. No existing review threads or blocking comments were present on refreshed intake. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and proof: ordinary INSERT VALUES/SELECT and UPDATE/MERGE predicates now consult pinned write defaults; the inline findings show incorrect typed-expression results and uncovered statement/schema paths. Added tests exercise the ordinary cases but do not prove the broader behavior.
- Scope, clarity, reuse: the capability-based change is focused and connector-agnostic. Reusing the default resolver loses referenced-column typing and reuses a narrower assignment-name validator.
- Concurrency and locks: no new threads or locks. Existing table read-lock/finally-unlock structure remains. Independently cached read schemas versus live writer schemas expose the reported type-evolution issue; no separate lock-order/deadlock defect found.
- Lifecycle and initialization: write schemas/identities remain statement-scoped; INSERT and row-level command pinning, retry/prepared-execution reset, plugin metadata scope and classloader boundaries were traced. No new lifetime/static-initialization defect found. Overwrite repinning behavior also exists for preexisting bare DEFAULT and is not a new finding here.
- Configuration: no configuration items added.
- Compatibility: the enum changes the plugin surface; the deliberate major API bump to 20.0, version pin and plugin-surface baseline agree. Metadata method signatures are unchanged. No new wire or storage format is introduced.
- Thrift/protobuf IDs: no gensrc definitions changed; branch field-ID checks are not applicable.
- Parallel paths: INSERT/OVERWRITE/EXPLAIN, UPDATE/MERGE/DELETE and CHANGELOG/POSITION_DELETE paths were traced. Row-level commands pin before planning. Aggregate/CTE/subquery normalization and snapshot Hive capability inheritance are incomplete as reported.
- Conditional checks: capability gates preserve ordinary Hive/Paimon/internal behavior; table-ID plus field-ID matching is consistent. Fully qualified target names are wrongly rejected as reported. A differing source alias is not raised because the PR deliberately specifies target-name semantics.
- Tests and modularity: added JUnit 5 cases cover direct VALUES/projection/no-FROM, unknown names, bound pin/absent pin, capability-disabled behavior and ordinary sibling delegation. Missing coverage includes typed nested expressions, grouped/nested queries, full qualification, evolved read/write types and snapshot sibling delegation.
- Changed expected results: the ordered Parquet/ORC rows 20-22 and expected SELECT rejection match the added statements and existing default value 35. Result-file generation provenance was not independently verified.
- Observability: existing analysis errors and statement tracing suffice for this planning change; no new distributed operation or metrics requirement identified.
- Persistence and failover: no EditLog/replay or durable metadata mutation protocol changes.
- Writes, atomicity and crashes: planning precedes existing writer transaction/finalization/commit fences. No new commit/rollback or crash-cleanup defect found; the P1 expression issue can persist the wrong computed value in an otherwise valid write.
- FE/BE state: no new transmitted variables or serialization/send paths. Integer-width function semantics were cross-checked in FE and BE for the P1 finding.
- Performance: traversal and default lookup add planning work proportional to query size and target width. No substantiated material CPU/memory regression or repeated external-I/O defect found.
- Other invariants: no BE allocation ownership, partition visible-version, or MoW delete-bitmap changes. Errors propagate through existing analysis exceptions. Stable connector field IDs and writer metadata fences remain in place, with the reported default-type caveat.
Validation was static only, as required: all 15 changed files, related callers/control flow, tests and expected outputs were inspected; no build, runtime test, source change, or format rewrite was performed. No unresolved candidate or assigned static coverage gap remains. Runtime execution and output-generation provenance remain unverified. Convergence and review severity are reported separately by the delivery helper.
Review converged after 2 round(s).
| if (!writeOnlyDefaultColumns.isPresent()) { | ||
| return value; | ||
| } | ||
| return (NamedExpression) ConnectorWriteSchemaUtils.resolveDefaultReferences(value, |
There was a problem hiding this comment.
[P1] Preserve the referenced column type before binding its consumers
For an Iceberg t(c INT) with write-default -1, INSERT INTO t(c) SELECT bit_count(DEFAULT(c)) should evaluate bit_count on an INT and write 32. This new route instead produces OneRow(bit_count(-1:TINYINT)): ConnectorWriteSchemaUtils.resolveDefaultReferences parses the default SQL without casting it to the referenced column type, and the parser chooses TINYINT for -1. BitCount has width-specific signatures, and both its FE evaluator and BE implementation return 8 for TINYINT -1, versus 32 for INT -1. Casting the completed expression to the sink's INT cannot restore the lost value; VALUES expressions have the same problem. Cast each substituted default to its referenced writer column's type before binding surrounding operators/functions, as the bound DEFAULT rewrite does.
There was a problem hiding this comment.
Fixed in cf75568. ConnectorWriteSchemaUtils.resolveDefaultReferences now casts each resolved default to the type of the writer column it references (TypeCoercionUtils.castUnbound) before the expression around it is bound, so bit_count(DEFAULT(c)) on an INT column with write-default -1 gets an INT -1 in VALUES and in SELECT. The external UPDATE / MERGE / changelog paths use the same helper and get the same typing. branch-4.2 is not affected: it builds write defaults as literals of the column type. Test: InsertUtilsTest#writeOnlyDefaultIsTypedAsItsColumnBeforeBinding (VALUES and SELECT); with the previous commit the argument of bit_count is TinyIntLiteral(-1).
| */ | ||
| private static Plan resolveSelectDefaultReferences(Plan query, Optional<List<Column>> writeOnlyDefaultColumns, | ||
| TableIf table, List<String> targetNameParts) { | ||
| return query.rewriteUp(plan -> { |
There was a problem hiding this comment.
[P2] Normalize defaults in every SELECT output shape and nested query
With target t(c INT) having a write-default, INSERT INTO t(c) SELECT DEFAULT(c) FROM (SELECT 1 AS k) s GROUP BY k parses as Sink(t.c) -> Aggregate(groupBy=k, output=DEFAULT(c)) -> Alias(s) -> OneRow(k=1). The lambda handles only Project/OneRow outputs, so it leaves this DEFAULT untouched and binding fails with unknown column c against source s.k; removing GROUP BY resolves the same target reference successfully. CTE definitions and scalar-subquery plans also escape this traversal: command-level WITH is attached after normalization, CTE definitions are extraPlans(), and subquery plans are not expression children. Resolve target defaults in aggregate/repeat outputs and explicitly traverse these separately owned query plans, with grouped/CTE/subquery coverage.
There was a problem hiding this comment.
Not changing this in this PR. The INSERT ... SELECT resolution covers the select lists of the main query (projects and the one-row relation), the same shapes as branch-4.2, which this PR ports (InsertUtils#resolveIcebergSelectDefaultReferences there). In an aggregate output, a WITH clause or a subquery, DEFAULT(c) keeps its meaning from before this PR and names a column of its own FROM clause, as in MySQL: with no such column the statement fails with an unknown-column error; a column of the target table itself gets the pinned writer default through the bound rewrite; a column of another Iceberg table now fails with "must reference a column of the table that this statement writes" instead of answering NULL; a column of a Hive or OLAP source gets that column's default, as before. Resolving target names in those shapes needs other analysis stages (the command-level WITH is attached after normalizePlan; CTE and subquery plans are not children of the project), so it would be a separate change.
| return value; | ||
| } | ||
| return (NamedExpression) ConnectorWriteSchemaUtils.resolveDefaultReferences(value, | ||
| writeOnlyDefaultColumns.get(), (ExternalTable) table, ConnectContext.get(), targetNameParts, null); |
There was a problem hiding this comment.
[P2] Accept fully qualified target column references
INSERT INTO ctl.db.t(c) SELECT DEFAULT(ctl.db.t.c) now reaches ConnectorWriteSchemaUtils.resolveDefaultReferences, which passes the four-part name to UpdateCommand.checkAssignmentColumn. That assignment-LHS validator accepts only one, two, or three name parts and throws column in assignment list is invalid for this unambiguous target reference. Ordinary Nereids column binding explicitly supports catalog.db.table.column, and this form should also resolve against the pinned target schema. Use qualification validation that includes the catalog component rather than applying the narrower UPDATE-assignment grammar; cover the fully qualified form in SELECT and VALUES.
There was a problem hiding this comment.
Not changing this here. DEFAULT(...) in VALUES / SELECT accepts the qualifications that the existing Iceberg UPDATE / MERGE DEFAULT handling accepts, through the same UpdateCommand.checkAssignmentColumn (branch-4.2 does the same): c, t.c, db.t.c. ctl.db.t.c fails with "column in assignment list is invalid" instead of resolving; it does not write a wrong value. Accepting catalog-qualified names would change the assignment-column rule of all DML paths, so it belongs in its own change.
| if (originalTable.filter(PluginDrivenExternalTable.class::isInstance) | ||
| .filter(table -> ((PluginDrivenExternalTable) table).hasWriteOnlyDefaults()) | ||
| .isPresent()) { | ||
| column = pinnedWriteColumn(context.cascadesContext.getStatementContext(), originalTable.get(), column) |
There was a problem hiding this comment.
[P2] Derive the result type after selecting the writer column
targetType is computed from the read column before this substitution and remains unchanged afterward. The read schema can still be cached as INT after an external Iceberg promotion to LONG, while pinAndGet independently loads current LONG writer columns and a new write-default such as 4900000001. Then UPDATE t SET other = 'x' WHERE DEFAULT(score) = 4900000001 produces Filter(CAST(4900000001 AS INT) = 4900000001) above the cached INT scan, instead of using the BIGINT writer default, causing overflow/error or an incorrect predicate. The writer fences compare the current writer columns/identity and do not validate this stale predicate type. Compute the type from the selected writer column and add a same-field-ID read-INT/write-BIGINT test.
There was a problem hiding this comment.
Fixed in cf75568. RewriteDefaultExpression now takes the result type from the pinned writer column, after the substitution. Test: RewriteDefaultExpressionTest#writeOnlyDefaultTakesTheTypeOfThePinnedWriterColumn pins a BIGINT writer column with default 4900000001 over an INT read column with the same field id and expects a BIGINT literal; with the previous commit the result is cast(4900000001 as INT).
There was a problem hiding this comment.
Follow-up: superseded in 688e8d5 (see the newer thread on this file). Typing the substituted default as the writer column fixed the DEFAULT node itself but not the expressions bound around it against the read type, so a writer column whose type differs from the read column now fails with a refresh hint instead.
| ConnectorCapability.SUPPORTS_STORAGE_PREDICATE_PRUNING, | ||
| ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE)); | ||
| ConnectorCapability.SUPPORTS_NESTED_COLUMN_SCHEMA_CHANGE, | ||
| ConnectorCapability.SUPPORTS_WRITE_ONLY_DEFAULTS)); |
There was a problem hiding this comment.
[P2] Reflect this capability for snapshot schemas as well
The two-argument getTableSchema reflects this set, but the snapshot overload still returns the sibling schema directly. For SELECT DEFAULT(c) FROM hms_catalog.db.iceberg_t FOR VERSION AS OF <snapshot_id>, PluginDrivenMvccExternalTable.loadSnapshot publishes that unreflected historical schema. tableCapabilities() reads the ambient pinned schema, while the Hive catalog itself does not declare write-only defaults, so hasWriteOnlyDefaults() is false and the bound rewrite returns NULL for a nullable column instead of rejecting this read-only use. Direct Iceberg catalogs retain the guard through their connector-wide capability. Apply the same reflection to getTableSchema(session, handle, snapshot) and test that overload.
There was a problem hiding this comment.
Not in this PR. HiveConnectorMetadata#getTableSchema(session, handle, snapshot) returns the sibling's schema without reflectSiblingCapabilities for every inherited capability, not only this one (auto-analyze, Top-N lazy materialization, nested column prune, field-id access path, storage predicate pruning, nested column DDL); that predates this PR. So for a time-travel read of an iceberg-on-HMS table, SELECT DEFAULT(c) ... FOR VERSION AS OF keeps master's current answer (NULL) instead of the new error; nothing is written. Reflecting capabilities on the snapshot path changes scan planning for all of them and should come with its own tests, so it is left to a separate change.
TPC-H: Total hot run time: 29120 ms |
TPC-DS: Total hot run time: 151076 ms |
ClickBench: Total hot run time: 24.29 s |
bda3e28 to
cf75568
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of all 16 changed paths at cf75568, including related binding, writer-schema, capability, and commit paths. One new P2 finding: the bound DEFAULT rewrite can widen its result after an enclosing IF has retained an INT signature, causing a valid BIGINT default to become NULL or overflow during folding.
The existing P1 4355508072 (parsed default width before binding) and the direct DEFAULT result-type instance in P2 4355508096 are fixed at this head. No existing P0/P1 remains applicable. Qualification (4355508088), grouped/CTE/subquery traversal (4355508080), and HMS historical-schema capability reflection (4355508100) remain covered by their existing threads and replies; they are not reposted. The new finding concerns a different narrowing cast synthesized later by the enclosing IF consumer.
Critical checkpoint conclusions:
- Goal and proof: writer-only defaults now use the target's pinned schema in the covered INSERT and row-level paths, and read-only use is rejected. Added tests cover these cases, but the enclosing-consumer promotion case in the inline finding remains incorrect.
- Scope and clarity: changes are localized and reuse the capability accessor, statement schema pin, and shared default resolver.
- Concurrency and locks: no new threads, shared mutators, locks, or lock order; existing statement ownership and connector generation fences remain in use. No new heavy work inside a newly introduced lock was found.
- Lifecycle and initialization: writer columns/identity are pinned before normalization or synthesis, selected by target table and field ID, and cleared across retries/executions. Branch selection remains in the existing writer scope. No static initialization dependency is introduced.
- Configuration: no configuration item is added.
- Compatibility: connector API major and test pin move from 20.0 to 21.0 with the new enum surface entry. Metadata method surface is unchanged. No storage-format or FE/BE protocol change.
- Thrift/protobuf: no gensrc definitions are changed.
- Parallel paths: inspected VALUES, SELECT projects/one-row plans, INSERT OVERWRITE, UPDATE/MERGE assignments and predicates, changelog helper consumers, and direct/HMS Iceberg delegation. Existing traversal/qualification/snapshot limits are identified above.
- Conditions and errors: capability gates preserve other connectors' behavior; absent matching target/field and invalid references fail explicitly. Optional absent Iceberg defaults serialize as SQL NULL; required absent defaults fail.
- Test coverage: positive/negative FE tests, capability/SPI tests, and Parquet/ORC regression additions cover the intended paths. Consumer binding plus rewrite under a stale read type needs coverage as requested inline. Tests were inspected, not executed.
- Expected results: both new ordered regression blocks match rows 20-22, default35, UPDATE labels and the MERGE change to row22. Result-generation provenance was not independently verified.
- Observability: existing analysis errors identify the DEFAULT reference/column; no new operational metric or log is needed for these local substitutions.
- Transactions and persistence: no EditLog, replay, failover or persistence algorithm changes; existing writer schema/identity checks and commit fences remain responsible for remote metadata races.
- Writes and atomicity: existing row-level transaction/commit machinery is reused. The inline finding concerns an incorrect pre-write predicate or analysis failure; no new crash/resource/partial-commit mechanism was identified.
- FE/BE variables: no new transported variable or sending path.
- Performance and memory: planning work is linear in visited plan/expressions with a writer-column lookup per DEFAULT; no new per-row runtime work or substantiated performance defect. No BE memory ownership, partition visible-version or MoW delete-bitmap code changes.
- Other issues and user focus: no additional user focus was supplied. All initial risks and candidates are accepted, duplicate-fenced or dismissed with recorded evidence.
Coverage limits: static inspection only, as required; no builds, unit/regression execution, live external-cluster behavior, or output-generation provenance were validated. No unresolved static candidate or unreviewed changed path remains. Convergence is reported separately by the delivery metadata.
Review converged after 2 round(s).
| } | ||
| // The pinned writer column can be wider than the read column the slot was bound with, e.g. after a type | ||
| // promotion committed outside Doris, and the default is a value of the writer column. | ||
| DataType targetType = DataType.fromCatalogType(column.getType()); |
There was a problem hiding this comment.
[P2] Bind enclosing consumers against the writer column type
With a cached read score INT and a same-field-ID writer score BIGINT whose write-default is 4900000001, consider:
UPDATE t SET score = DEFAULT(score)
WHERE IF(TRUE, DEFAULT(score), 0) = 4900000001;Binding produces Filter(CAST(IF(TRUE, DEFAULT(score:INT), 0):INT AS BIGINT) = 4900000001) -> Scan(score:INT). This rewrite correctly replaces DEFAULT with a BIGINT literal, but Rewriter preserves function signatures, so If.withChildren/FunctionParams retain IF's INT result. FoldConstantRuleOnFE.visitIf then calls ensureSameResultType on the true branch and synthesizes CAST(4900000001 AS INT): loose casting yields NULL and updates no rows, while strict casting throws overflow. The predicate should be true for every row.
The earlier direct DEFAULT result-type issue is fixed here; this narrowing cast is created separately by the already-bound IF consumer. Resolve the pinned writer type before binding its consumers, or rebind their signatures/coercions after substitution, and cover an enclosing IF through both analysis and rewrite.
There was a problem hiding this comment.
Fixed in 688e8d5. Agreed: once IF, CASE or COALESCE have been bound against the read type, a wider writer-typed default cannot be substituted from the DEFAULT node, because the rewriter keeps their signatures and constant folding narrows the value back. The read and writer types of one field only differ while Doris's cached read schema predates a type change committed outside Doris (reads go through the table cache, the writer schema is loaded for the statement), so the bound rewrite now fails such a DEFAULT and asks to refresh the table instead of retyping its consumers. When the two types agree, which is every case without an outside type change, nothing changes. DEFAULT in VALUES, in INSERT ... SELECT projections and in UPDATE SET is resolved against the writer schema before binding, so its consumers already bind against the writer type. RewriteDefaultExpressionTest covers a direct DEFAULT and one under an IF bound to INT; both are rejected.
TPC-H: Total hot run time: 28331 ms |
TPC-DS: Total hot run time: 150589 ms |
ClickBench: Total hot run time: 24.43 s |
FE Regression Coverage ReportIncrement line coverage |
cf75568 to
f1287fd
Compare
|
run buildall |
TPC-H: Total hot run time: 28559 ms |
TPC-DS: Total hot run time: 151566 ms |
ClickBench: Total hot run time: 24.6 s |
FE UT Coverage ReportIncrement line coverage |
|
/review |
There was a problem hiding this comment.
Reviewed exact head 688e8d59412ab4269516ff421a22015d5df4e281 across all 17 changed files and related planning/write paths. No new inline findings. Existing P1 #4355508072 is fixed: each substituted default is typed as its referenced writer column before consumer binding. Existing type-drift findings #4355508096 and #4358048801 are fixed by rejecting mismatched read/writer types with a refresh hint.
Known P2 limits remain covered by existing threads, without duplicate comments: #4355508080 (Aggregate/Repeat outputs and separately owned CTE/scalar-subquery plans), #4355508088 (four-part target qualification), and #4355508100 (historical Iceberg-on-HMS capability reflection). These are verified dispositions, with no unresolved new candidate. No existing P0/P1 finding still applies. No additional user focus was supplied.
Critical checkpoint conclusions:
- Purpose and proof: ordinary VALUES/SELECT projections and bound-target UPDATE WHERE/MERGE ON now use the pinned writer default. Inspected unit/regression additions cover these paths; the known scope limits above remain.
- Focus and reuse: the change is localized and reuses the capability union, pinned writer schema, shared DEFAULT resolver and coercion helpers; generic planning stays connector-agnostic.
- Concurrency and locks: no new threads, lock acquisitions or shared mutable state; rewriting consumes the existing statement pin in the established planning lock scope. No new lock-order or concurrency defect found.
- Lifecycle: INSERT, overwrite and row-level writes pin before normalization; branch selection survives overwrite's inner INSERT. Prepared execution/retry reset clears writer schemas and identities. No new static-initialization or resource lifecycle.
- Configuration: no configuration change.
- Compatibility: the added capability is recorded in the plugin surface and the API major is consistently 21.0. ConnectorMetadata methods and storage/wire formats are unchanged.
- Thrift/protobuf: no definitions changed; cross-branch field-ID comparison is inapplicable.
- Parallel paths: explicit/implicit VALUES columns, FROM/no-FROM projections, overwrite/EXPLAIN, UPDATE, MERGE and shared changelog callers were traced. Plain Hive/Paimon/internal paths retain their existing non-opt-in behavior.
- Conditions and failures: missing target fields and mismatched writer types fail with context. Stable field-ID lookup avoids selecting a replacement field solely by reused name; target-name SELECT semantics are explicit and tested.
- Test coverage: inspected positive/negative tests cover capability declaration/reflection, writer-versus-read defaults, unknown columns, non-target rejection, non-opt-in behavior, width-sensitive numeric typing, and direct/enclosing type drift. End-to-end additions cover Parquet and ORC.
- Expected results: the new ordered rows 20/21/22 with default_int=35 and UPDATE/MERGE strings agree with the fixture and statements by inspection. Output-generation provenance and runtime passing status were not independently verified.
- Observability: analysis errors identify the DEFAULT reference or conflicting types and refresh action. No new distributed operation needs additional metrics/logging.
- Persistence and failover: no EditLog, replay or transaction-state change; existing writer metadata-identity/schema fences and commit paths remain in use.
- Writes and atomicity: changed expressions flow into existing analyzed connector sinks; no new commit, crash cleanup or partial-write mechanism. No new atomicity issue found.
- FE/BE propagation: no new transmitted variable or operator.
- Performance: one plan traversal and bounded linear column lookup per DEFAULT; no new per-row evaluation or concrete significant regression found.
- Core invariants and remaining issues: no new visibility-version, delete-bitmap or native memory-ownership path; errors fail analysis before execution. Final recheck found no additional substantiated issue beyond the existing threads.
Validation was static-only: no builds, tests, cluster execution or source modifications. Runtime behavior, full plugin packaging and test-generation provenance remain outside this invocation's validation.
Review converged after 2 round(s).
FE Regression Coverage ReportIncrement line coverage |
…hema ### What problem does this PR solve? Issue Number: None Related PR: apache#66538, apache#57531, apache#65851 Problem Summary: An Iceberg write-default exists only in the writer schema that a write statement pins (IcebergWriteSchemaContext); the columns of the read schema carry no default. Master resolves DEFAULT(column) against that writer schema only in UPDATE / MERGE SET and in MERGE INSERT VALUES, and everywhere else falls back to the read column. On a v3 table whose column d has write-default 42, INSERT ... VALUES (DEFAULT(d)) and INSERT ... SELECT DEFAULT(d) without FROM fail with "Unknown column", INSERT ... SELECT DEFAULT(d) FROM t writes NULL, UPDATE ... WHERE d = DEFAULT(d) and MERGE ... ON ... d = DEFAULT(d) compare d with NULL and silently match no row, and SELECT DEFAULT(d) FROM t returns NULL. branch-4.1 and branch-4.2 (apache#66538) resolve all of these against the pinned write target, by name against the target in INSERT ... SELECT, and reject DEFAULT(column) on an Iceberg column in a statement that does not write the table. This change adds the connector capability SUPPORTS_WRITE_ONLY_DEFAULTS, declared by the Iceberg connector and inherited by iceberg-on-HMS tables. For a table that declares it, InsertUtils resolves DEFAULT(column) in INSERT VALUES and in the select lists of INSERT ... SELECT by name against the pinned writer schema, and RewriteDefaultExpression takes the default of a DEFAULT(column) bound to the table from the pinned writer column with the same field id, which covers the UPDATE and MERGE conditions, and rejects it when the statement pinned no writer schema for the table. Hive, Paimon and OLAP tables keep the current behavior. The new enum constant changes the connector plugin API surface, so the plugin API version goes from 20.0 to 21.0. ### Release note Fix: On Iceberg tables, DEFAULT(column) in INSERT VALUES, INSERT ... SELECT and the WHERE / ON conditions of UPDATE and MERGE now resolves to the column's write-default. Before, it failed, wrote NULL, or matched no row. SELECT DEFAULT(column) on an Iceberg table now fails instead of returning NULL. ### Check List (For Author) - Test: Regression test / Unit Test - test_iceberg_initial_defaults: writes rows 20-22 with DEFAULT(default_int) in VALUES and in INSERT ... SELECT with and without FROM, then updates them with UPDATE ... WHERE default_int = DEFAULT(default_int) and MERGE ... ON ... = DEFAULT(...), for Parquet and ORC; SELECT DEFAULT(default_int) must fail. Passes locally; with master's FE it fails at the first INSERT with "Unknown column 'default_int'" - InsertUtilsTest: VALUES and INSERT ... SELECT (from a staging relation and without FROM) resolve against the pinned writer schema; unknown column fails; a table without the capability is unchanged - RewriteDefaultExpressionTest: pinned writer column wins, no pinned schema fails, a table without the capability keeps the read default - IcebergConnectorTest, HiveConnectorMetadataSiblingDelegationTest, ConnectorPluginSurfaceTest - Behavior changed: Yes. See the release note. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Two review findings on the previous commit. ConnectorWriteSchemaUtils.resolveDefaultReferences parsed the default SQL of a writer column and substituted it without a cast, so the default kept the type the parser gives the literal: -1 is a TINYINT. For an INT column with write-default -1, INSERT INTO t(c) VALUES (bit_count(DEFAULT(c))) and INSERT INTO t(c) SELECT bit_count(DEFAULT(c)) evaluated bit_count on a TINYINT and wrote 8 instead of 32; casting the finished value to INT cannot restore that. The external UPDATE, MERGE and changelog paths use the same helper and typed DEFAULT(column) the same way. The resolved default is now cast to its column's type before the expression around it is bound, as RewriteDefaultExpression does for a bound DEFAULT(column). branch-4.2 builds write defaults as literals of the column type and is not affected. RewriteDefaultExpression took the result type from the read column before substituting the pinned writer column. After a type promotion committed outside Doris, the read schema can still hold an INT column while the writer column is a BIGINT whose default an INT cannot hold, such as 4900000001, and the default was cast to INT. The type now comes from the writer column the default is taken from. ### Release note None ### Check List (For Author) - Test: Unit Test - InsertUtilsTest#writeOnlyDefaultIsTypedAsItsColumnBeforeBinding: VALUES and SELECT pass DEFAULT(bits) to bit_count as an INT; with the previous commit it is a TINYINT - RewriteDefaultExpressionTest#writeOnlyDefaultTakesTheTypeOfThePinned WriterColumn: an INT read column with a BIGINT pinned writer column gives the BIGINT default; with the previous commit it is cast(4900000001 as INT) - InsertUtilsTest, RewriteDefaultExpressionTest, ConnectorWriteSchemaUtilsTest, ConnectorChangelogPlanBuilderTest, ChangelogRowLevelDmlTransformTest, ExternalRowLevelUpdatePlanBuilderTest, ExternalRowLevelMergePlanBuilderTest, PluginDrivenExternalTableTest: 93 tests pass - Behavior changed: No. DEFAULT(column) inside an expression of an external UPDATE / MERGE / changelog assignment now has the column's type, which is the type a bound DEFAULT(column) already had. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: apache#69394 added two ConnectorWriteSchemaUtilsTest cases that expect resolveDefaultReferences to return the default with the type the parser gives its literal (9 is a TINYINT). The previous commit casts the resolved default to its writer column's type, so for the INT test column the result is now an INT literal 9, and both cases failed in FE UT once this branch met master. The two expectations now use the column's type. ### Release note None ### Check List (For Author) - Test: Unit Test - ConnectorWriteSchemaUtilsTest, RewriteDefaultExpressionTest, InsertUtilsTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
… read type ### What problem does this PR solve? Issue Number: None Related PR: apache#66538 Problem Summary: Review follow-up. The previous commits typed the default that replaces a bound DEFAULT(column) as its pinned writer column. That type differs from the read column's only when the table changed outside Doris after Doris cached its read schema, e.g. an INT column promoted to BIGINT whose new write-default is 4900000001. The expressions around DEFAULT(column) were already bound against the read type and the rewriter keeps their signatures, so in UPDATE t SET ... WHERE IF(TRUE, DEFAULT(score), 0) = 4900000001 the IF keeps its INT result type and constant folding casts 4900000001 back to INT: NULL in non-strict mode, so no row matches, and an overflow error in strict mode. A substituted value cannot repair the types its consumers were bound with, so the rewrite now fails such a DEFAULT and asks to refresh the table. When the two types agree, which is every case without a type change made outside Doris, nothing changes. ### Release note None ### Check List (For Author) - Test: Unit Test - RewriteDefaultExpressionTest: a DEFAULT whose writer type differs is rejected, directly and under an IF bound to the read type - ConnectorWriteSchemaUtilsTest, InsertUtilsTest, ExternalRowLevelUpdatePlanBuilderTest, ExternalRowLevelMergePlanBuilderTest, ChangelogRowLevelDmlTransformTest, PluginDrivenExternalTableTest - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
688e8d5 to
676a924
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Request changes: one new P1 finding. INSERT ... SELECT normalization crosses a FROM-derived query boundary and can silently write the target default in place of the source default. The inline comment includes the concrete SQL and reduced plan. All 17 changed files and their relevant upstream/downstream paths were reviewed statically.
Critical checkpoint conclusions:
- Goal/data correctness: pinned writer defaults fix the covered simple INSERT, UPDATE and MERGE cases, but the derived-source regression prevents full correctness.
- Scope/clarity/reuse: the capability and shared resolver keep the change focused and connector-agnostic.
- Concurrency/locks: no new thread, lock or shared mutable global; statement pins and existing generation fences remain in use. No new locking defect found.
- Lifecycle/static initialization: writer columns remain statement-owned and are cleared before retry/re-execution; connector resources close through existing scope cleanup. No new ownership or static-order issue.
- Configuration: none added.
- Compatibility: the appended SPI capability is recorded in the plugin surface; API property and test both advance to 21.0. No metadata-method signature or storage-format change.
- Thrift/protobuf: no changes.
- Parallel paths: checked explicit/implicit VALUES, SELECT, overwrite/retry/EXPLAIN, UPDATE, MERGE, DELETE and shared changelog callers; direct Iceberg and ordinary HMS delegation are covered. Known historical HMS limitation remains as noted below.
- Conditional checks/error handling: missing writer pins/fields and read/write type mismatches fail analysis; capability guards have concrete purpose. The accepted source-scope defect bypasses the intended source interpretation.
- Tests: new unit/regression cases cover typed substitution, unknown columns, capability opt-out, stale types, Parquet/ORC writes and read-only rejection; derived-source defaults lack coverage.
- Expected results: new output rows are consistent with the SQL on static inspection; output generation and runtime success were not independently verified.
- Observability: existing analysis errors carry the DEFAULT expression and type/refresh context; no additional long-lived operation requires metrics.
- Persistence/failover: no new edit-log or persistence path; existing Iceberg identity/schema/spec/order fences remain unchanged.
- Writes/atomicity: existing connector commit/abort machinery is reused; the new P1 concerns the value supplied to an otherwise atomic write.
- FE/BE variables: none added; existing expression serialization is reused.
- Performance: work is proportional to plan/expression size and schema lookup width during planning; no new per-row execution work or substantiated performance regression.
- Additional concerns/focus: no additional user focus was supplied; all initial risk questions have explicit dispositions and no new candidate remains unresolved.
Existing-thread dispositions: P1 r4355508072 is fixed by casting each default to its referenced writer-column type before binding, so existing_blocking_comment_ids is empty. P2 r4355508096 and r4358048801 are fixed by rejecting a writer/read type mismatch before substitution. The aggregate/CTE/scalar-subquery limitation (r4355508080), four-part qualification limitation (r4355508088), and historical HMS capability-reflection limitation (r4355508100) remain already-reported P2 concerns and are not reposted. The new finding is the opposite scope failure: traversal enters a FROM-derived query and captures a valid source reference.
Review converged after three rounds over the frozen finding set, with complete assigned static coverage. No builds or tests were run, as required by this review environment; runtime behavior and generated-output provenance remain outside this static review's validation scope.
Review converged after 3 round(s).
| */ | ||
| private static Plan resolveSelectDefaultReferences(Plan query, Optional<List<Column>> writeOnlyDefaultColumns, | ||
| TableIf table, List<String> targetNameParts) { | ||
| return query.rewriteUp(plan -> { |
There was a problem hiding this comment.
[P1] Preserve source defaults across derived-query boundaries
Let OLAP src(c INT DEFAULT 7) contain one row and Iceberg target t(c INT) have pinned write-default 9:
INSERT INTO t(c)
SELECT v FROM (SELECT DEFAULT(c) AS v FROM src) q;The reduced input plan is Sink(t.c) -> Project(v) -> SubQueryAlias(q) -> Project(DEFAULT(c) AS v) -> Scan(src.c). query.rewriteUp descends through q and resolves the inner projection against the outer writer columns before source binding, producing Project(9:INT AS v) and silently writing 9 instead of the source default 7. Writing DEFAULT(src.c) makes the same valid source query fail instead: checkAssignmentColumn compares src with target t. This is distinct from the existing unvisited aggregate/CTE/scalar-subquery issue: here an independently scoped FROM query is traversed and its source reference is overwritten. Stop target-default substitution at derived-query boundaries while retaining the outer query's output/set-operation handling, and cover both the qualified source reference and different source/target defaults.
TPC-H: Total hot run time: 28152 ms |
TPC-DS: Total hot run time: 150311 ms |
ClickBench: Total hot run time: 24.29 s |
FE Regression Coverage ReportIncrement line coverage |
What problem does this PR solve?
Issue Number: None
Related PR: #66538, #57531, #65851
Problem Summary:
In short. On an Iceberg table,
DEFAULT(column)anywhere exceptUPDATE ... SETandMERGE ... VALUESreads the default from the read schema, which carries no Iceberg write-default. This PR resolves it against the write schema that the statement pinned for its target, as branch-4.1 and branch-4.2 do.Background. An Iceberg
write-defaultapplies to rows written after it is set. So it lives in the writer schema that a write statement pins (IcebergWriteSchemaContext), not in the columns of the read schema. #65851 resolvesDEFAULT(column)against that pinned schema inUPDATE ... SET,MERGE ... UPDATE SETandMERGE ... INSERT VALUES(ConnectorWriteSchemaUtils). Every otherDEFAULT(column)either fails at binding or reachesRewriteDefaultExpression(#57531), which reads the default from the bound column. branch-4.1 #66538, also in branch-4.2, resolves these against the pinned write target too. It rejectsDEFAULT(column)on an Iceberg column in a statement that does not write the table.The problem, and what it cost. Reproduced on a local cluster with a
file://format-version 3 table:CREATE TABLE t (id INT, c INT),ALTER TABLE t ADD COLUMN d INT DEFAULT 42, one row(0, 0), sod = 42.INSERT INTO t (id, c, d) VALUES (1, 1, DEFAULT(d))Unknown column 'd' in 'table list'INSERT INTO t (id, c, d) SELECT 2, 2, DEFAULT(d) FROM t WHERE id = 0INSERT INTO t (id, c, d) SELECT 3, 3, DEFAULT(d)Unknown column 'd'UPDATE t SET c = 100 WHERE d = DEFAULT(d)d = NULLmakes the plan an empty setd = 42is pushed down to the scanMERGE INTO t USING (SELECT 0 AS id) s ON t.id = s.id AND t.d = DEFAULT(t.d) WHEN MATCHED THEN UPDATE SET c = 300SELECT DEFAULT(d) FROM tDEFAULT(d) must reference a column of the table that this statement writesThe
UPDATEandMERGEcases fail silently: the statement succeeds and changes nothing.How this PR fixes it.
SUPPORTS_WRITE_ONLY_DEFAULTSsays which kind a table is. The Iceberg connector declares it. The Hive connector passes it on to iceberg-on-HMS tables, like the other per-table capabilities.InsertUtils.normalizePlanresolvesDEFAULT(column)inINSERT ... VALUESand in the select lists ofINSERT ... SELECTagainst the pinned write schema. It does this by name, before the source relations are bound, so the name always refers to a target column, as in branch-4.2. In MySQL,DEFAULT(column)in aSELECTrefers to a source column instead.RewriteDefaultExpressiontakes the default for aDEFAULT(column)bound to such a table from the pinned write column with the same field id. That covers theWHEREofUPDATEand theONofMERGE. If the statement pinned no write schema for that table, it fails.RewriteDefaultExpressionalready does for a boundDEFAULT(column). The parsed literal-1is a TINYINT, so without the castbit_count(DEFAULT(c))on an INT column would count 8 bits instead of 32. The externalUPDATE,MERGEand changelog paths use the same helper.RewriteDefaultExpressionrejects a boundDEFAULT(column)whose writer column has another type than the read column the statement was bound with, which happens only when the table changed outside Doris after Doris cached it, and asks to refresh the table: the expressions around it were bound against the read type.connector.plugin.api.versiongoes from 20.0 to 21.0, as for the other recent changes to that surface ([feature](iceberg) Support whole-file metadata delete for DELETE #69176 took 20.0).Results.
EXPLAIN UPDATE t SET c = 5 WHERE d = DEFAULT(d)goes from an empty set to a scan withd = 42pushed down to Iceberg.InsertUtilsTestand 5 in the newRewriteDefaultExpressionTest. They cover VALUES, and INSERT ... SELECT both from a staging relation whose own column is alsonameand without FROM: each resolves to the pinned default, and an unknown column fails. The pinned write column wins over the read column. A statement that writes nothing fails. A table without the capability is unchanged. With master'sInsertUtilsandRewriteDefaultExpression, the 4 tests of the fix fail and the 2 tests of the unchanged path pass.IcebergConnectorTest,HiveConnectorMetadataSiblingDelegationTestandConnectorPluginSurfaceTestpass. So do the fe-core tests around the write schema:ConnectorWriteSchemaUtilsTest,ExternalRowLevelUpdatePlanBuilderTest,ExternalRowLevelMergePlanBuilderTest,ChangelogRowLevelDmlTransformTestandPluginDrivenExternalTableTest. Two of the tests pin the typing:bit_count(DEFAULT(bits))gets the default as an INT in VALUES and in SELECT, and fails without the cast; a BIGINT writer column over an INT read column is rejected, directly and under an IF bound to INT.test_iceberg_initial_defaultsnow writes rows 20 to 22 withDEFAULT(default_int)in VALUES and in INSERT ... SELECT with and without FROM. It then updates them withWHERE default_int = DEFAULT(default_int)andMERGE ... ON ... = DEFAULT(...), for Parquet and ORC, and checks the rows in the.out. A plainSELECT DEFAULT(default_int)must fail. The suite passes locally. With master's FE it fails at the first of these statements:Unknown column 'default_int' in 'table list'.Release note
Fix: On Iceberg tables,
DEFAULT(column)inINSERT ... VALUES,INSERT ... SELECTand theWHERE/ONconditions ofUPDATEandMERGEnow resolves to the column's write-default. Before, it failed, wrote NULL, or matched no row.SELECT DEFAULT(column)on an Iceberg table now fails instead of returning NULL.Check List (For Author)
Test
On a local cluster with a
file://Iceberg catalog:CREATE TABLE t (id INT, c INT) PROPERTIES ('format-version'='3'),ALTER TABLE t ADD COLUMN d INT DEFAULT 42, then the statements in the table above.Behavior changed:
See the release note.
INSERT ... SELECT DEFAULT(column)into an Iceberg table names a target column, even when the source has a column of that name.Does this need documentation?
Check List (For Reviewer who merge this PR)