Repository navigation
[fix](ivm) Use type zero values for missing hidden slots - #69268
Merged
yujun777 merged 2 commits intoOct 21, 2026
Merged
Conversation
yujun777
requested review from
924060929,
englefly,
morrySnow and
starocean999
as code owners
October 19, 2026 07:31
Contributor
Author
|
run buildall |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
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.
Reviewed all 46 changed paths at bf20210. Review converged in 2 rounds: both scope reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS against the unchanged final inline set. One P2 defect is reported: the new public default-value API throws for valid DECIMALV2(p,p). No P0/P1 finding or existing blocking inline comment was identified. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal: the DATE/DATETIME hidden-slot planning failure is addressed by exact-typed, non-null defaults; source sequence masking keeps these placeholders separate from MV ordering and row identity.
- Scope and reuse: the broad type edits implement one coherent API using existing literal and empty-state function classes; the existing IVM dispatch is centralized.
- Concurrency and locking: no shared mutation, thread, lock, blocking work or lock-order change is introduced.
- Lifecycle and initialization: defaults are created on demand; no new resource ownership, release obligation or static initialization cycle was found.
- Configuration: no configuration item or dynamic-update contract changes.
- Compatibility: concrete type inheritance is covered; no persisted format, backend function symbol or rolling-upgrade wire contract changes.
- Thrift/protobuf: no definitions or field IDs change.
- Parallel paths: checked incremental delta remapping, pre/post snapshots, full-refresh and sink defaults, aggregate/plain MOW and append-only DUP behavior, and supported join alternatives. Unsupported mark/semi/anti joins remain rejected.
- Conditions and failure handling: non-hidden-slot and ROW_LSN rejection remain intact; unavailable defaults still fail with slot/type context. The decimal API defect is described inline with its bounded impact.
- Test coverage: new unit assertions directly cover DATEV2 and DATETIMEV2(6) defaults. The regression uses strict INCREMENTAL, requires task SUCCESS and disables transparent rewriting; it covers both join sides' updates and simultaneous new keys. It cannot pass by automatic full-refresh fallback. Negative cases and the wider type matrix are not exhaustively covered by the new tests.
- Expected results: every new output row matches the SQL mutations, with ordered results. Generation provenance of the .out file was not independently verified.
- Observability: existing IVM exception/task diagnostics remain appropriate; this expression-construction change introduces no operational state requiring new metrics.
- Transactions, persistence and failover: no journal, stream-offset, version or commit/publish state changes; existing visibility and MoW transaction mechanisms remain in place.
- Writes, atomicity and crashes: no new write protocol, partial-commit path or crash-cleanup resource is introduced.
- FE/BE propagation: existing literal serialization retains concrete types/scales, including temporal values; no new transmitted variable is needed.
- Performance: construction remains bounded per planned hidden slot, with no added data scan, per-row work or external IO.
Coverage limits: this was a static review; no build, unit test, regression run or cluster execution was performed. The regression places the sequence-bearing table on the logical left, so it reads that table's post snapshot and does not force the new temporal pre-snapshot placeholder end to end. A reversed logical join would strengthen that coverage; the helper is directly covered by the unit assertions. Other temporal scales/legacy types and outer-join/delete variants were traced statically. These are disclosed validation limits; no unresolved correctness candidate remains beyond the verified inline P2. The P2 concerns the newly documented type API; no current decimal-hidden-slot SQL refresh failure is claimed.
Review converged after 2 round(s).
|
|
||
| @Override | ||
| public Optional<Expression> defaultValue() { | ||
| return Optional.of(new DecimalLiteral(this, BigDecimal.ZERO)); |
Contributor
There was a problem hiding this comment.
[P2] Scale decimal zero before validating the default literal
DecimalV2Type.createDecimalV2Type(9, 9).defaultValue() throws AnalysisException instead of returning zero. BigDecimal.ZERO has precision 1 and scale 0, and DecimalLiteral.checkPrecisionAndScale validates it before the constructor rescales it, so its integer width (1) exceeds precision - scale (0). This affects valid DECIMALV2(p,p) defaults requested through the new API; the former IntegerLiteral(0).castTo(type) scaled zero first. Construct zero at getScale() before passing it to DecimalLiteral, and cover the precision-equals-scale case. This is an API-contract defect; the current IVM hidden-slot path does not produce decimal columns.
Contributor
Author
There was a problem hiding this comment.
Fixed: zero now carries the type scale before DecimalLiteral validates it (BigDecimal.ZERO.setScale(getScale())), and the same fix is applied to DecimalV3Type. IvmUtilTest now covers DECIMAL(9,9) for both V2 and V3.
Also reversed the regression join so the sequence-bearing table is the right child; a delta on the left dimension then makes it the pre-snapshot side, so the suite exercises the temporal placeholder end to end.
yujun777
force-pushed
the
fix/ivm-hidden-slot-date-default
branch
from
October 19, 2026 08:26
bf20210 to
f00f625
Compare
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
No new actionable findings at f00f625a287be0bdf78795137fbb534f338152db. Reviewed all 46 changed paths, their production callers and literal translation. Two complete-scope reviewers and one independent risk reviewer returned NO_NEW_VALUABLE_FINDINGS; both initial risk questions were dismissed with evidence.
The concrete refresh path is Delta(dim) JOIN PreSnapshot(datetime-sequenced base). Snapshot remapping must construct the missing DATETIMEV2 hidden-slot default before the enclosing normalized projection masks the source sequence. The new typed literal satisfies that requirement without replacing the visible seq_dt, row identity or IVM's separately allocated BIGINT sequence. Both new regression MVs place the datetime base on the right to exercise this branch.
The existing decimal default finding is fixed: both decimal implementations scale zero before construction, and the unit test invokes V2/V3 DECIMAL(9,9) defaults. No existing P0/P1 finding remains to carry forward. No additional user focus was supplied.
Critical checkpoint conclusions:
- Goal and evidence: the temporal placeholder resolves the identified planning failure; DATEV2/DATETIMEV2 unit assertions and aggregate/plain regression scenarios target the changed behavior. Execution was not performed.
- Scope and abstraction: explicit per-type implementations centralize value construction; IvmUtil retains hidden-column policy. No unrelated source change identified.
- Concurrency and locking: no new threads, shared mutable state, metadata locks or lock-order changes.
- Lifecycle and initialization: no new owned resources or static initialization dependency; expressions are constructed on demand.
- Configuration: no production configuration added; regression settings disable transparent SELECT and DML rewrites.
- Compatibility: no persistent layout or protocol change; concrete type descendants implement or inherit the added Java method. Existing literal representations are reused.
- Thrift/protobuf IDs: no gensrc changes; cross-branch field-ID comparison is inapplicable.
- Parallel paths: incremental delta, pre/post snapshot, complete refresh, BindSink and append-only detail paths were traced; common defaults share this API.
- Conditions and failure behavior: unknown hidden slots, mandatory ROW_LSN mapping, special overrides and absent type defaults retain explicit failure semantics. The append-only ROW_LSN exception remains model-specific.
- Test coverage: JUnit 5 checks selected defaults and decimal precision-equals-scale; the integration suite covers dimension updates, greater source sequence updates and pending inserts on both sides. Explicit INCREMENTAL disallows silent fallback after the established baseline.
- Test results: all eight ordered output blocks agree with the SQL operations by static analysis. Runtime success and automatic-generation provenance were not independently verified.
- Observability: existing PLAN_REWRITE_FAILED errors retain slot/type context and task failure diagnostics; no additional instrumentation is needed for constant construction.
- Transactions, persistence and failover: no edit-log, stream-offset, baseline publication or transaction operation changed.
- Data writes and correctness: source placeholders remain outside business values, row identity and sink ordering; no change to visible-version or delete-bitmap handling was identified.
- FE-BE propagation: no new transmitted variable; existing literal-to-legacy and Thrift conversion paths preserve concrete type parameters.
- Performance: construction adds constant planning work and avoids the former numeric/string cast path; no added row-wise loop, scan or multiplicative plan expansion.
- Further recheck: reviewed parameterized lengths/scales, temporal minima, JSON versus SQL NULL, empty collection types, descendant completeness and source/sink sequence separation; no additional verified issue.
Review converged in round 1 with zero new inline findings and no unresolved static candidate or uncovered changed path. This was a static-only review: no builds, tests or source modifications. The tests sample the new default API and do not exhaust every type, temporal precision, NULL, deletion or lower-sequence case; runtime validation remains outside this invocation.
Review converged after 1 round(s).
…lots An IVM base table can carry a DATETIME sequence column through function_column.sequence_col, which makes its hidden __DORIS_SEQUENCE_COL__ a DATETIMEV2 slot. The incremental rewrite asks for the hidden slot's default value when a join's snapshot side does not produce the column, but the default-value handling only covered numeric, string-like and bitmap types, so planning failed with "no default value for IVM hidden slot: __DORIS_SEQUENCE_COL__, type=DATETIMEV2(6)". Key changes: - Add an abstract DataType#defaultValue() and implement it in every concrete type with that type's non-null default value. - Resolve common hidden slot defaults through DataType#defaultValue() in IvmUtil instead of branching on type categories. - Add an IVM regression suite covering a DATETIME sequence column with aggregate and plain join materialized views. Unit Test: - IvmUtilTest#testCommonHiddenSlots - IvmUtilTest#testDefaultValueForEveryType - regression-test/suites/mtmv_p0/ivm/test_ivm_datetime_sequence.groovy
yujun777
force-pushed
the
fix/ivm-hidden-slot-date-default
branch
from
October 19, 2026 09:35
f00f625 to
e777b7b
Compare
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
No new actionable findings in the static review of all 46 changed paths at e777b7b. Review converged in 1 round: two complete-review agents and a separate temporal-risk agent returned NO_NEW_VALUABLE_FINDINGS on the unchanged empty finding set; all candidates and risk items have evidence-based dispositions.
The existing decimal-zero finding is corrected: both decimal implementations scale zero before construction, and the unit test covers DECIMAL(9,9). There are no applicable existing P0/P1 comments to carry forward. The focus file contains no additional user concerns.
| Required critical checkpoint | Conclusion |
|---|---|
| Goal and supporting test | Typed DATE/DATETIME defaults address the missing snapshot-sequence planning failure. The revised dimension-left/base-right join reaches the temporal pre-snapshot fallback; unit tests inspect type, scale and date components. |
| Scope and clarity | The common fallback delegates to a consistent type API; all 41 added implementations and the DataType contract were checked. The storage-column masking change is explanatory text. |
| Concurrency and locking | No new threads, shared mutable state, locks or lock-order change. Existing refresh synchronization is unchanged. |
| Lifecycle and initialization | Defaults are built on demand with existing literal/function constructors; no new static initialization dependency or resource lifetime. |
| Configuration | No new configuration or dynamic-update requirement. |
| Compatibility | Concrete subclasses implement or inherit the new internal API. No storage/wire format change; existing prerelease IVM compatibility policy applies. |
| Thrift/protobuf ids | No gensrc definition changes. |
| Parallel paths | Incremental snapshot alignment, complete refresh, sink defaults and append-only DUP paths share the helper. Both join orientations and source/MV sequence separation were traced. Cloud/non-cloud refresh orchestration is unchanged. |
| Special conditions and errors | Common-slot validation, the explicit missing-ROW_LSN failure, and exceptional-default precedence remain intact. Unsupported defaults still raise the contextual IvmException. |
| Test coverage | JUnit additions cover temporal defaults and decimal(p,p). The regression covers aggregate/plain joins, dimension updates, higher-sequence base upserts and both-side insertion; it disables SELECT/DML MV rewrites and uses strict INCREMENTAL refresh. |
| Expected test results | All eight ordered output blocks match the SQL data changes by inspection. The wait helper requires task SUCCESS; strict INCREMENTAL does not silently fall back to COMPLETE. |
| Observability | Existing planning errors retain slot/type context and failure classification; existing refresh/task logging is sufficient for the new path. |
| Transactions and persistence | No EditLog, offset persistence or failover logic changed. Snapshot capture and success-only snapshot/epoch recording remain in existing orchestration. |
| Writes and atomicity | Placeholder construction only changes planning; existing insert transaction, MoW and crash-cleanup behavior is preserved. |
| FE/BE transmission | No new transmitted fields. Temporal literal copying/folding/translation retain their concrete type and scale; canonical year-zero date parsing was checked. |
| Performance | Fixed-size literal/empty-collection construction adds no scans, RPCs, row loops or unbounded allocation. |
| Other invariants | No new data-visibility, row-identity, delete-bitmap, ownership or silent-error defect established. A decimal256/session-gating candidate was dismissed because the former cast path used the same gated constructor and no newly affected supported caller was found. |
Limits: no builds or tests were run, as required by this review environment. Runtime behavior and output-generation provenance are unverified. The new regression exercises DATETIMEV2(6) inner joins; DATEV2, other scales and reverse join orientation were traced statically, and restart/crash cases were not executed. No unresolved static correctness candidate or unreviewed changed path remains.
Review converged after 1 round(s).
Contributor
Author
|
run p0 |
Contributor
TPC-H: Total hot run time: 28637 ms |
Contributor
TPC-DS: Total hot run time: 150850 ms |
Contributor
ClickBench: Total hot run time: 24.07 s |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
Author
|
run external |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
TPC-H: Total hot run time: 28365 ms |
Contributor
TPC-DS: Total hot run time: 150187 ms |
Contributor
FE UT Coverage ReportIncrement line coverage |
Contributor
ClickBench: Total hot run time: 24.42 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
FE Regression Coverage ReportIncrement line coverage |
1 similar comment
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run cloud_p0 |
Contributor
FE Regression Coverage ReportIncrement line coverage |
Contributor
Author
|
run external |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
previously approved these changes
Oct 20, 2026
Distinguish synthesized type zero values from SQL column DEFAULT values. Key changes: - Rename DataType.defaultValue() to zeroValue() - Document the expression contract and cover JSON null Unit Test: - tools/fast-compile-fe.sh - tools/fast-fe-ut.sh IvmUtilTest - mvn checkstyle:check -pl fe-core
yujun777
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Reviewed all 46 changed paths at 38b4936. No new actionable findings. The existing decimal precision-equals-scale finding (thread 4311624787) is fixed: both decimal variants scale zero before validation, with DECIMAL(9,9) unit coverage. No existing P0/P1 finding remains to carry forward. No additional user focus was supplied.
The main review and three reviewers covered the complete type API, IVM integration/tests, and independent temporal/snapshot risks. All three returned NO_NEW_VALUABLE_FINDINGS against the same empty inline set. Review converged in 1 round, with no unresolved static candidate or uncovered changed path.
Critical checkpoint conclusions:
- Goal and evidence: type-owned non-null defaults fix the missing temporal sequence placeholder during scan replacement. For the regression's dimension-left/base-right join, a left delta reads preSnapshot(base); the adapter constructs the typed placeholder with the original ExprId, and the normalization project removes the source sequence before handlers select the MV's own BIGINT sequence. Unit tests and aggregate/plain regression cases cover this intended path; reviewed statically.
- Scope and reuse: the 42 type/coercion edits implement the new abstract method using existing literals/functions; IvmUtil centralizes consumption. No unrelated source change found.
- Concurrency and locking: no new threads, shared mutable state, lock acquisition, or lock-order changes. Expressions are created during planning.
- Lifecycle and static initialization: no new resource ownership, cached expression lifecycle, or static initialization dependency.
- Configuration: no configuration added or dynamic-update requirement introduced.
- Compatibility: concrete DataType subclasses implement or inherit the API; no persisted schema/storage format or mixed-version protocol change. Existing IVM compatibility policy applies.
- Thrift/protobuf: no definitions changed.
- Parallel paths: incremental scan remapping, pre/post snapshots, full-refresh stream replacement, unique-key sinks and append-only DUP sinks checked. Mandatory real row-LSN mapping remains intact.
- Conditions and failures: common-hidden-slot admission, row-LSN refusal and unsupported-type failures remain explicit; no newly silent business/identity default found.
- Test coverage: JUnit additions cover DATEV2/DATETIMEV2 and decimal boundaries plus representative zero values. Regression covers aggregate/plain MVs, dimension-only delta, sequenced base update and simultaneous new-key insertion. Both transparent rewrites are disabled; strict INCREMENTAL requests cannot hide a planning failure through COMPLETE fallback. Unchanged negative paths and remaining type variants were checked by static control-flow/constructor review.
- Expected outputs: all eight new result blocks match the SQL/DML and use deterministic order_qt output. Result-file generation provenance was not verified.
- Observability: existing contextual IvmException/task failure reporting covers these planning errors; no new operation needs metrics or logging.
- Transactions and persistence: no EditLog, offset-commit, failover, or snapshot-lifecycle behavior changes.
- Writes and crash behavior: existing refresh transaction boundaries remain; the MV retains its delta sequence and computed delete sign, and business/identity values remain sourced from scans.
- FE/BE values: existing typed literal/function translation and serialization reused; temporal scales and ranges checked, including UTC timestamp and nanosecond epoch defaults. No new transmitted field.
- Performance: constant-size expression creation per missing slot during planning; no new per-row work, scan, RPC, or unbounded computation found.
- Other mechanisms: exact collection/character/binary types, non-null JSON null, empty state functions, concrete type admission, outer-join repair and chained MVs checked. The existing Decimal256 session gate is unchanged and no new admitted hidden-decimal path was found. No additional substantiated issue.
Validation limits: static review only, as required. No builds or tests were run, and runtime behavior or .out generation is not claimed verified. No remaining static coverage gap.
Review converged after 1 round(s).
Contributor
TPC-H: Total hot run time: 28616 ms |
Contributor
TPC-DS: Total hot run time: 150423 ms |
Contributor
ClickBench: Total hot run time: 24.37 s |
Contributor
FE Regression Coverage ReportIncrement line coverage |
morrySnow
approved these changes
Oct 21, 2026
Contributor
|
PR approved by anyone and no changes requested. |
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
hello-stephen
pushed a commit
that referenced
this pull request
Oct 21, 2026
Related PR: #69311, #69268, #63639 Problem Summary: #69311 renamed the row binlog (the change stream IVM reads) to **changelog** and rejected the legacy keys `binlog.format` / `binlog.need_historical_value` in every DDL. Two test files merged just before it fell outside that rename sweep and now fail: 1. `regression-test/suites/mtmv_p0/ivm/test_ivm_datetime_sequence.groovy` (from #69268) — creates its IVM base tables with `"binlog.enable" = "true", "binlog.format" = "ROW", "binlog.need_historical_value" = "true"`, which CREATE TABLE now rejects. 2. `RelationImpliedPredicateTest#testIvmOutputPredicatesSkipHiddenColumns` (from #63639) — same legacy properties in the base-table DDL, and it gates the creation on `enable_feature_binlog` instead of the new `enable_feature_changelog`. Fix: switch both to `"changelog.enable" = "true"` (plus `"changelog.need_historical_value" = "true"` where the before-image is needed), drop the rejected `binlog.format`, and use `enable_feature_changelog` as the feature gate in the FE unit test. Related IVM trace issue: #65418 .
OIiveirra
added a commit
to OIiveirra/doris
that referenced
this pull request
Oct 21, 2026
### What problem does this PR solve? Issue Number: None Related PR: apache#69032 Problem Summary: Upstream apache#69268 adds the abstract DataType.zeroValue interface. GeometryType and GeographyType omitted it, causing both TeamCity COMPILE 1076409 and performance 1076365 to fail during FE compilation of the merged PR. Return Optional.empty for both types, retaining the lack of a default non-null spatial expression. Add a consumer-level unit test covering default/custom CRS and Geography algorithms: hidden-slot default generation fails explicitly instead of synthesizing a spatial value. No spatial defaults, internal storage or IVM capability are added. ### Release note None ### Check List (For Author) - Test: Unit Test / Manual test - JDK17 FE UT 92/92: DataTypeTest, IvmUtilTest, SpatialPredicateLegalityTest, SpatialAggregateLegalityTest and ConnectorSpatialWriteValidatorTest. - Standard build.sh --fe -j16 and Checkstyle passed on the merged official master; build hygiene passed. - Existing ordinary-key spatial-value admission controls remain passing. No service restart, external SQL, local BE UT or performance test was needed for this planner interface compatibility fix. - Behavior changed: No (existing spatial semantics retained; unavailable zero values are explicit) - Does this need documentation: No
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?
Related issue: #65418
Problem Summary:
An IVM base table can carry a DATETIME sequence column through
function_column.sequence_col. The table's hidden__DORIS_SEQUENCE_COL__is then a DATETIMEV2 slot. During an incremental refresh a join's snapshot side does not produce that column, and the rewrite must synthesize its type's zero value. The previous handling only covered numeric, string-like, and bitmap types, so planning failed with:This is independent of cloud mode: a base table with a DATETIME sequence column and a join in the MV query reproduces it on a shared-nothing cluster.
Fix:
DataTypenow declareszeroValue(), which returns a non-SQL-NULL, deterministic expression without slot references when the type has a safe value to synthesize. Concrete types supply zero values such as0,'',0000-01-01 00:00:00,0.0.0.0, JSONnull, and empty bitmap/HLL values. Types without a safe value returnOptional.empty(), causing IVM planning to fail loudly.IvmUtil.getCommonHiddenSlotDefaultuses this API instead of branching on type categories.Release note
Fix an IVM defect where a base table with a DATETIME sequence column could not be incrementally refreshed when the MV query contained a join.
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)