Skip to content

[fix](fe) Keep decimal precision of MAP key/value and scalar Any groups independent in default decimal v3 promotion - #67154

Draft
starocean999 wants to merge 9 commits into
apache:masterfrom
starocean999:master_0527
Draft

[fix](fe) Keep decimal precision of MAP key/value and scalar Any groups independent in default decimal v3 promotion#67154
starocean999 wants to merge 9 commits into
apache:masterfrom
starocean999:master_0527

Conversation

@starocean999

@starocean999 starocean999 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Nereids' default DECIMALV3 precision promotion
(ComputeSignatureHelper.defaultDecimalV3PrecisionPromotion) recursively collects every
DECIMALV3 in the argument types, computes a single global "wider" type, and replaces ALL
decimal slots with it via TypeCoercionUtils.replaceDecimalV3WithTarget. This merges
independent type variables and breaks correctness in several places:

  1. MAP<K, V> key/value slots are independent but get merged into one wider type.
    Widening the key's scale to match the value overflows a big integral key, so
    MAP_KEYS (and other map consumers) silently turns legal keys into NULL. UNNEST(MAP)
    (rewritten to EXPLODE_MAP) then fails type analysis because the promoted MAP type no
    longer matches the input.
  2. Nested containers lose the enclosing MAP path: for a nonconstant
    m MAP<DECIMAL(38,0), ARRAY<DECIMAL(38,18)>>, the array leaf is merged globally with
    the key and becomes DECIMAL(38,6) under the default overflow scale, so
    map_values(m) discards low-order fractional digits through a lossy cast. Nested MAP
    recursion can similarly apply an outer-key lookup promotion to an unrelated inner key.
  3. map_agg/map_agg_v2 expose their MAP key/value as independent top-level Any(0)/
    Any(1) arguments; the fallback merged them to DECIMAL(38,6), so a DECIMAL(38,0)
    key is cast to a type with only 32 integral digits (entry disappears as a NULL key)
    and twelve fractional digits are truncated from a DECIMAL(38,18) value before
    aggregation.

Fix: group-based promotion. collectDecimalLeaf/replaceDecimalV3Leaf carry a full
structural path through nested containers (key, value, value/array, value/key),
so MAP-nested leaves aggregate only with leaves on the same path and stay independent
across different paths. Top-level scalar slots form logical groups by their resolved type
(scalarGroupWider), so the independent Any groups of map_agg/map_agg_v2 keep their
own precision/scale. A top-level scalar linked to a MAP leaf by resolved type (e.g.
element_at's lookup) folds into the outermost matching MAP leaf group. All other leaves
(unlinked scalars, top-level array items, NULL slots, varargs) and the placeholder return
type keep the original single-wider-type behavior.

None

Check List (For Author)

  • Test

    • Regression test
    • Unit Test
    • Manual test (add detailed scripts or steps below)
    • No need to test or manual test. Explain why:
      • This is a refactor/code format and no logic has been changed.
      • Previous test can cover this change.
      • No code files have been changed.
      • Other reason
  • Behavior changed:

    • No.
    • Yes.
  • Does this need documentation?

    • No.
    • Yes.

Check List (For Reviewer who merge this PR)

  • Confirm the release note
  • Confirm test cases
  • Confirm document
  • Add branch pick label

@hello-stephen

Copy link
Copy Markdown
Contributor

Thank you for your contribution to Apache Doris.
Don't know what should be done next? See How to process your PR.

Please clearly describe your PR:

  1. What problem was fixed (it's best to include specific error reporting information). How it was fixed.
  2. Which behaviors were modified. What was the previous behavior, what is it now, why was it modified, and what possible impacts might there be.
  3. What features were added. Why was this function added?
  4. Which code was refactored and why was this part of the code refactored?
  5. Which functions were optimized and what is the difference before and after the optimization?

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fixed-argument MAP path now preserves independent DecimalV3 leaves, and the added unit/regression expectations are deterministic and internally consistent. However, the shared promotion helper still introduces three blocking correctness failures: linked Any/Follow slots can diverge across Decimal storage widths; field can retain different representations for its fixed operand and repeated search operands; and complex vararg signatures still collapse independent nested leaves into one global type. The two inline comments cover those three root causes.

Checkpoint summary: the four-file scope is focused, but the generic helper affects many built-ins and extension functions. Parallel Follow consumers and scalar/complex vararg paths were traced through FE cast insertion and BE dispatch. NULL/empty inputs and all Decimal32/64/128/256 boundaries were checked. There are no new concurrency, lifecycle, static-initialization, configuration-definition, transaction, persistence, data-write, wire/protocol, rolling-upgrade, observability, or status-propagation surfaces; the bounded signature-tree traversal has no actionable performance concern. Test coverage is useful but omits all three blocking triggers. No build or test command was run because this runner's review instructions prohibit them. No additional reviewer focus was supplied. The three-round review converged with no unresolved or undeduplicated candidates.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

Blocking findings:

  • The new inline comment shows a distinct nested-container failure: dropping the enclosing MAP path makes MAP<DECIMAL(38,0), ARRAY<DECIMAL(38,18)>> cast its value leaf to DECIMAL(38,6), silently discarding fractional digits before map_values or other default-compute consumers execute.
  • The previously reported logical Any/Follow group issue is resolved/outdated on GitHub but remains incomplete on this head. When independent MAP key/value groups resolve to the same Decimal type, the key-first association at lines 607-618 assigns a value-following map_contains_value/map_contains_entry operand to the key group. A MAP<DECIMAL(9,2),DECIMAL(9,2)> plus a VARCHAR value lookup therefore produces expected inputs with a Decimal128 RHS but a Decimal32 map value, and BE rejects or miscasts the typed comparison. I did not duplicate the inline because this is the same logical-group root cause. The direct FIELD/repeated-MAP examples from the other prior thread appear corrected, though their new tests do not actually repeat the MAP vararg or exercise a nonconstant cross-width FIELD path.

Checkpoint conclusions:

  • Goal and proof: direct MAP key/value separation, Decimal32-to-Decimal64 key lookup, and NULL lookup are covered, but the generic promotion goal is not achieved for logical-group collisions or supported nested values.
  • Scope and clarity: the four-file scope is localized, but two maps keyed by resolved Decimal type plus a global fallback do not model logical or structural type-variable identity clearly enough.
  • Concurrency: no threads, locks, shared mutable state, or atomic ordering are involved.
  • Lifecycle/static initialization: no special ownership, teardown, circular reference, or cross-TU/static-order surface applies.
  • Configuration: no configuration item is added; existing decimal_overflow_scale only makes the demonstrated narrowing deterministic (default 6).
  • Compatibility: there is no wire, symbol, or storage-format change, but the current coercions are query-semantics incompatible because they can lose data or send different concrete Decimal classes to typed BE comparisons.
  • Parallel paths and conditions: ElementAt, all MapContains* functions, MapKeys, MapValues, DeduplicateMap, MapSize/Cardinality, ExplodeMap, varargs, NULL, arrays, and nested maps were traced. The mapSide reset/overwrite and key-first scalar condition are the unsafe branches. Custom/exact paths such as MapFromArrays, MapEntries, and Unnest opt out where appropriate.
  • Tests and results: the changed Groovy/output files are deterministic and follow the regression conventions, and their expected rows match the queries. They omit the two blocking triggers and the repeated/nonconstant production cases above. No build or test command was run because the bundle explicitly prohibits builds. Current live checks show style/license/secret checks passing, but no FE unit or regression execution proving these cases.
  • Observability: no new logging or metric is needed for this analysis-time helper.
  • Transactions, persistence, and data writes: no EditLog, failover, transaction, or storage-write path is changed.
  • FE/BE variables and protocol: no transmitted field is added; the relevant risk is existing FE expected-input types versus BE typed execution.
  • Performance: traversal and grouping are linear in arguments/type leaves; no separate actionable performance issue was found.
  • Error handling and other issues: no ignored Status or exception-boundary change is introduced, but the surviving logical-group defect reaches a BE runtime error instead of a valid comparison result.

Review completion: Round 1 produced the nested-container candidate; Round 2's two full reviewers and separate risk reviewer all returned NO_NEW_VALUABLE_FINDINGS. Every candidate is accepted, dismissed with concrete evidence, or deduplicated. No additional reviewer focus was supplied.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

…ts aligned in default decimal v3 precision promotion

The previous per-slot promotion kept MAP key/value independent but broke
other scenarios (field's fixed first operand + vararg tail comparison) and
still merged linked Any/Follow slots (e.g. element_at's MAP key and lookup).

Redesign defaultDecimalV3PrecisionPromotion to be group based:
- Non-MAP decimal slots (scalar, ARRAY item, vararg scalar) keep the original
  behavior of using the single wider type across all decimal slots, so field
  and other scalar/array functions are not affected.
- MAP key/value leaves are independent groups keyed by their resolved leaf
  type: key and value never merge, while the corresponding leaves across
  different (or repeated/vararg) MAP arguments aggregate; a NULL MAP argument
  falls back to its own group's wider type.
- A top-level scalar slot with a concrete resolved type that matches a MAP
  key/value leaf (Any/Follow linked, e.g. element_at/map_contains_key lookup)
  is promoted together with that leaf, so the BE compares columns of the same
  concrete decimal class across the DECIMAL32/64 boundary and for NULL.

Tests: updated ComputeSignatureHelperTest (non-MAP back to wider type, NULL
MAP uses group wider) + new tests for vararg MAP, field vararg, element_at
linked slots and NULL lookup; regression suite extended with storage-width
boundary, NULL lookup and field cases.
…n default decimal v3 precision promotion

For a nonconstant MAP<DECIMAL(38,0), ARRAY<DECIMAL(38,18)>> the previous
recursion dropped the incoming MAP_VALUE when entering an ARRAY, so the array
item was merged globally with the key and became DECIMAL(38,6) under the
default overflow scale, letting expectedInputTypes() insert a lossy cast that
discarded low-order fractional digits before map_values() executed. Nested MAP
recursion overwrote the outer branch too and could apply an outer-key lookup
promotion to an unrelated inner key.

Carry the full structural/logical group path (e.g. key, value, value/array,
value/key) through ARRAY/MAP recursion instead of a flat key/value marker:
- ARRAY items nested in a MAP value keep the enclosing value group, so
  MAP<K, ARRAY<D>> no longer truncates the item to the global wider type.
- the key/value of a nested MAP stay on their own path and never merge with
  the outer leaves, and an outer-key linked scalar promotion cannot leak to an
  unrelated inner key of the same resolved type.
- leaves outside any MAP (top-level scalar, ARRAY item) keep the original
  single wider-type behavior.

Tests: 3 new unit tests for nested ARRAY in MAP value, nested MAP, and
outer-key linked scalar not promoting an unrelated inner key; regression suite
extended with nonconstant nested-container columns (map_values/map_keys over
MAP<DECIMAL(38,0), ARRAY<DECIMAL(38,18)>> and MAP<DECIMAL(38,0),
MAP<DECIMAL(9,2), DECIMAL(5,2)>>), .out regenerated and verified stable.
@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

Blocking findings:

  • The new inline comment covers a distinct parallel path: map_agg and map_agg_v2 expose independent MAP key/value variables as top-level scalar arguments, but the helper still merges all top-level Decimal slots into one widerType, causing overflow/truncation before aggregation.
  • The previously reported logical Any/Follow group issue also remains on this head. When independent MAP key/value groups resolve to the same Decimal type, key-first mapLeafGroupByType association can route a value-following map_contains_value/map_contains_entry operand to the key group, leaving Decimal32 versus Decimal64 comparison inputs. I did not duplicate the inline because the exact current mechanism is already documented in the prior REQUEST_CHANGES review body and has the same root cause as that thread.
  • The same existing thread explicitly covers the surviving NullType identity loss: a NULL MAP leaf can fall back to an unrelated global Decimal group and change a NULL-key lookup. This is likewise summary-only to avoid duplication.

Checkpoint conclusions:

  • Goal and proof: direct MAP key/value separation, linked key lookup, NULL lookup, varargs, and nested MAP/ARRAY inputs have useful tests, but the generic logical-group goal is incomplete because the value-linked and aggregate paths above remain incorrect.
  • Scope and clarity: the four-file change is localized, but this shared signature helper affects many built-ins; reconstructing identity from concrete Decimal equality and structural position is not sufficient for all logical groups.
  • Concurrency: no threads, locks, shared mutable state, or atomic-ordering behavior is involved.
  • Lifecycle/static initialization: no special ownership, teardown, circular-reference, or cross-TU initialization surface applies.
  • Configuration: no configuration item is added; existing decimal_overflow_scale=6 makes the demonstrated DECIMAL(38,6) narrowing deterministic when Decimal256 is disabled.
  • Compatibility: there is no wire, symbol, storage-format, or rolling-upgrade change. The blockers are query-semantic and FE/BE concrete-type compatibility failures.
  • Parallel paths and conditions: ElementAt, all MapContains* functions, MapKeys/MapValues, nested containers, varargs, CreateMap, MapFromArrays, MapFromEntries, and both MAP aggregate variants were traced. The unsafe branches are type-keyed scalar linking, global fallback for empty groups, and global promotion of independent top-level scalar groups.
  • Tests and results: the changed unit assertions and 12 regression outputs are deterministic and internally consistent for their covered paths. They omit nonconstant map_contains_value/map_contains_entry equal-type collisions and Decimal key/value map_agg coverage. No build or test command was run because the bundle explicitly prohibits builds; live style, license, secret, and formatting checks pass, but no FE unit or regression check is present.
  • Observability: no new logging, VLOG, metric, or tracing is needed for this analysis-time helper.
  • Transactions, persistence, and writes: no EditLog, failover, transaction, storage-write, or atomicity path is changed.
  • FE/BE variables and protocol: no transmitted field is added; the issue is that FE expected-input casts can hand BE mismatched or already-narrowed Decimal columns.
  • Performance and error handling: traversal is bounded by arguments/type leaves and no separate performance issue was found; no Status or exception boundary is changed, although the logical-group defect can surface as a BE runtime type error.
  • Additional reviewer focus: none was supplied.

Review completion: Round 1 produced one duplicate-fenced logical-group candidate and the distinct aggregate candidate. In Round 2, two full reviewers and the separate risk-focused reviewer all returned NO_NEW_VALUABLE_FINDINGS after independently verifying both blockers and rechecking the dismissed hypotheses. Every candidate is accepted, deduplicated, or dismissed with concrete evidence; no unresolved suspicious point remains.

@starocean999 starocean999 changed the title [fix](fe) Keep MAP key/value decimal slots independent in default deimal v3 precision promotion [fix](fe) Keep decimal precision of MAP key/value and scalar Any groups independent in default decimal v3 promotion Aug 27, 2026
@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions

Copy link
Copy Markdown
Contributor

Codex automated review failed and did not complete.

Error: Codex completed, but no new pull request review was submitted for the current head SHA.
Workflow run: https://github.com/apache/doris/actions/runs/33055960312

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

Blocking findings:

  • [P1] Equal resolved Decimal types still collapse independent MAP key/value groups, so a value-following probe can be rebound to the key group and reach BE with incompatible concrete Decimal columns.
  • [P2] The aggregate regression invokes MapAggV2 twice and never exercises MapAggV1.
  • [P2] The duplicate-key aggregate regression asserts a winner that SQL input/partial-merge order does not guarantee.

Checkpoint conclusions:

  • Goal and proof: the PR aims to preserve independent MAP Decimal groups while retaining linked comparison groups. Direct key/value, nested, vararg, NULL, and aggregate cases have useful coverage, but the goal is not achieved because the value/entry collision above remains.
  • Scope and clarity: the production change is localized to FE signature precision plus CreateStruct, but reconstructing logical identity from resolved Decimal equality at mapLeafGroupByType is not sufficient. CreateStruct's exact dynamic argument/return signature makes its precision opt-out safe.
  • Concurrency: all new state is per-expression and local; no threads, locks, atomics, or shared mutable state are involved.
  • Lifecycle/static initialization: no ownership, teardown, circular-reference, or cross-TU/static-initialization surface applies.
  • Configuration: no configuration item is added or dynamically changed.
  • Compatibility: no function symbol, wire protocol, storage format, or rolling-upgrade behavior is changed. Mismatched FE expected-input types versus BE Decimal column dispatch are nevertheless a current semantic/runtime compatibility failure. Cloud and shared-nothing behavior are not split here.
  • Parallel paths and conditions: ElementAt, MapContainsKey, MapContainsValue, MapContainsEntry, nested/vararg MAPs, scalar groups, CreateStruct, and both MAP aggregate implementations were traced. Key-following paths are preserved; value/entry paths fail when independent groups resolve equally. The key-first putIfAbsent condition/comment does not encode the original Any/Follow group identity.
  • Tests and results: the changed unit and scalar regression expectations are coherent for covered cases. Coverage omits the nonconstant equal-resolved value/entry collision, the advertised V1/V2 pair executes V2 twice, and the duplicate-key output is nondeterministic. No local build or test was run because this review runner explicitly prohibits it. Live style, formatter, license, secret, and large-file checks pass; no FE unit or regression-test result is exposed for this head.
  • Error handling, memory safety, and nullability: no Status/exception boundary, allocator ownership, reservation, or BE nullable conversion is modified. The accepted implementation defect can surface as a BE runtime type error rather than a valid comparison.
  • Observability: this analysis-time helper needs no additional logging, metrics, or tracing.
  • Transactions, persistence, and data writes: no EditLog, failover, transaction, atomic-write, or storage mutation path is changed.
  • FE/BE variables and protocol: no new transmitted field is added; the issue is the existing expected-input contract yielding mismatched concrete Decimal columns.
  • Performance: traversal remains linear in arguments/type leaves with small local maps; no separate CPU, memory, or complexity issue was found.
  • Other issue: the aggregate test comment claims last-value overwrite, while both implementations keep the first duplicate encountered.
  • User focus and completion: no additional reviewer focus was supplied. Round 2's two complete reviewers and separate risk reviewer all returned NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, dismissed with concrete evidence, or duplicate-fenced, and no suspicious point remains unresolved.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

Blocking findings:

  • A container-level Any(0) that resolves to MAP loses its identity when recursion descends into key/value leaves at different depths. ARRAY-of-MAP push operations can therefore retain incompatible MAP types and fail or corrupt Decimal values in BE insertion.
  • The scalar-only fallback separates a Decimal probe from the non-MAP ARRAY item in the same Any/Follow group. Existing ARRAY regressions already contain cross-width inputs that this head rewrites to incompatible concrete Decimal types.

Checkpoint conclusions:

  • Goal and scope: the patch fixes the covered direct/nested MAP key/value cases and the repaired aggregate tests are coherent, but the shared precision helper is not behavior-preserving for the two ARRAY topologies above. CreateStruct's exact dynamic argument/return signature makes its precision opt-out consistent with CreateNamedStruct.
  • Expected inputs, returns, and compatibility: both blockers were traced through Any resolution, precision replacement, expectedInputTypes(), Follow-based return propagation, and BE execution. They cause query-semantic/runtime incompatibility without changing symbols, storage formats, wire fields, or rolling-upgrade protocols.
  • Parallel paths and conditions: scalar ARRAY consumers (array_contains, array_position, countequal, array_remove, push front/back, and array_apply), container MAP mutation, nested/repeated containers, varargs, NULL, integral literals, and Decimal32/64/128 boundaries were checked. No third distinct production topology survived.
  • Tests: the changed unit/regression results are deterministic and repair the prior V1/V2 and duplicate-key issues, but omit reverse-widening ARRAY/scalar and ARRAY-of-MAP plus separately typed MAP inputs. Existing Array.groovy already exercises the first failing direction. No build or test was run because the bundle prohibits it; live style, formatter, license, secret, and large-file checks pass, while no FE unit/regression result is exposed.
  • Concurrency, lifecycle, configuration, persistence, transactions, data writes, observability, and static initialization: no new surface applies; all grouping state is invocation-local. The bounded analysis-time traversal has no separate actionable performance issue. The accepted defects reach runtime errors or wrong-scale values rather than a new error-handling boundary.
  • User focus and completion: no additional focus was supplied. After the complete seven-file diff review, Round 2's two normal reviewers and risk reviewer all returned NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, dismissed with evidence, or duplicate-fenced.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

New finding:

  • The inline comment identifies a regression fixture that narrows an eighteen-integral-digit value into DECIMAL(9,3). That row becomes NULL or errors before array_contains can exercise the claimed cross-width path, and the suite has no positive equality case proving exact promotion semantics.

Previously raised production findings remain hard duplicate fences, particularly the whole-complex-container and ARRAY/scalar logical-group discussions; I did not repeat them inline.

Checkpoint conclusions:

  • Goal and proof: the change aims to keep independent Decimal groups separate while co-promoting linked Any/Follow slots. Direct and nested MAP paths, scalar/ARRAY links, varargs, NULL, aggregate V1/V2, Struct precision, and Follow-based returns were traced. The new fixture does not fully prove the repaired ARRAY behavior, and existing production discussions remain part of the required-change context.
  • Scope and clarity: changes are confined to Nereids signature precision, CreateStruct's exact-precision opt-out, and targeted unit/regression coverage. The helper is complex because it reconstructs logical groups through nested type trees, but no additional scope issue survived review. No separate reviewer focus was supplied.
  • Concurrency and lifecycle: all new grouping state is invocation-local. No threads, locks, atomics, shared mutable state, special ownership, teardown, or static-initialization concern applies.
  • Configuration and compatibility: no configuration, wire protocol, storage format, symbol, or rolling-upgrade behavior changes. The relevant contract remains FE expected-input typing versus BE concrete Decimal dispatch.
  • Parallel paths and conditions: checked element_at, MAP contains/extraction/construction, ARRAY comparison/mutation/apply, field fixed and repeated arguments, MAP aggregates, nested containers, equal resolved types, NULL, literals, fixed/expanded varargs, and exact Struct paths. Remaining whole-container concerns were deduplicated against the existing threads.
  • Tests and results: unit and regression additions otherwise follow repository ordering/output conventions, and aggregate V1/V2 plus deterministic-key coverage is repaired. CheckStyle, formatter, license, secret, and large-file checks are green on c1a67df59806077d6d622a03350df17adb3d8932; build and functional checks are skipped or absent. I did not run builds or tests locally because this review contract prohibits them.
  • Error handling, observability, persistence, transactions, and writes: no Status/exception boundary, logging/metric requirement, EditLog, failover, transaction, or storage-write path is changed.
  • Performance: the added work is planner-time traversal linear in argument and type-tree size; no execution hot-path regression was identified.

Review completion: two complete rounds converged. Round 2's normal and risk-focused reviewers both returned NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, deduplicated against live threads, or dismissed with concrete code evidence.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current head still requires changes.

Blocking findings:

  • A NULL indexed MAP key/value leaf is not registered in its Any/Follow group, so a concrete linked probe can retain a different Decimal storage type and fail in BE before NULL is examined.
  • An Any/Follow slot that resolves to ARRAY-of-MAP loses its container identity at ARRAY recursion, splitting linked nested MAP operands into incompatible expected-input types.

Checkpoint conclusions:

  • Goal and proof: the patch correctly preserves the covered direct/nested MAP groups, scalar/ARRAY links, aggregate groups, and exact Struct fields, but the two valid-call topologies above still violate its central co-promotion contract.
  • Scope, minimality, and clarity: the production changes are localized to Nereids signature precision and the focused CreateStruct opt-out. The remaining defects are in the generic grouping traversal/registration rather than unrelated scope.
  • Concurrency, lifecycle, and static initialization: all added collections are invocation-local during expression analysis; no threads, locks, atomics, shared mutable state, ownership, teardown, or initialization-order surface applies.
  • Configuration and compatibility: no production configuration, persisted metadata, function symbol, wire/storage format, or rolling-upgrade behavior changes. The blockers are query-semantic and FE expected-input versus BE concrete-Decimal compatibility failures.
  • Parallel paths, inputs, returns, and conditions: MAP key/value/entry lookup, ElementAt, ARRAY comparison/mutation functions, fixed and expanded varargs, both MAP aggregates, nested containers, resolved-type collisions, NULL, and Decimal32/64/128/256 boundaries were traced. Follow-based returns are rebuilt after precision processing; the unsafe conditions are the NULL MAP-leaf registration gate and loss of container identity through ARRAY recursion.
  • Tests and result files: the changed unit and regression coverage is deterministic, query/output labels align, both aggregate implementations are exercised, and the repaired ARRAY fixture is representable. It does not cover these two remaining nonconstant triggers. No build or test command was run because the review bundle explicitly prohibits builds and tests.
  • Error handling, memory safety, and nullability: no Status/exception boundary or ownership behavior changes, but both accepted defects turn valid calls into BE type errors or wrong-scale insertion; the NULL case fails before runtime null semantics can apply.
  • Observability: no new logging, metric, or tracing is warranted for this analysis-time helper.
  • Transactions, persistence, writes, FE/BE variables, and protocol: no transaction, journal, failover, storage-write, transmitted-variable, or protocol surface applies.
  • Performance: the added signature scan and type-tree grouping are bounded per expression and linear in arguments/leaves; no separate actionable performance issue was found.
  • Reviewer focus and completion: no additional focus was supplied. Round 2 converged when both full reviewers and the separate risk reviewer returned NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, dismissed with evidence, or duplicate-fenced.

@starocean999

Copy link
Copy Markdown
Contributor Author

/review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review conclusion: approval opinion for the current head; no new blocking issue was found.

Reviewed head: 33925f824c1c431cfabd260ad23256ac8ffdfbc0

Critical checkpoint conclusions:

  • Goal and evidence: the change keeps independent DecimalV3 leaves in MAP/complex signatures separate while preserving shared Any/Follow groups, and opts struct(...) out of cross-field precision promotion. The focused unit cases and end-to-end regression suites cover map keys/values/entries, linked lookups/probes, nested ARRAY/MAP containers, NULL evidence, Decimal32/64/128/256 boundaries, struct fields, and both MAP aggregate implementations.
  • Scope and clarity: the production changes are confined to signature precision reconstruction plus the CreateStruct precision override. The recursive identity machinery is sizable, but collection and replacement use mirrored structural/index keys and the reviewed parallel paths remain consistent.
  • Concurrency and lifecycle: this is analysis-time local state only. It introduces no shared mutable state, locks, threads, special lifecycle, static-initialization dependency, or resource ownership change.
  • Configuration and compatibility: no configuration, dynamic-reload behavior, function symbol, FE-BE variable, wire/storage format, or rolling-upgrade contract changes are introduced.
  • Parallel paths and conditions: MAP key/value, ARRAY item/scalar, whole-container Any, nested containers, NULL leaves/containers, expanded varargs, map scalar/aggregate variants, and the parallel struct/map constructors were traced. Preconditions and conditional fallbacks preserve the existing failure model; no silent error continuation was added.
  • Tests and expected results: all 27 order_qt_ labels match one generated output block, multirow results are ordered, fixtures are representable, and the earlier aggregate alias/duplicate-winner and Decimal(9,3) fixture issues are repaired. No builds or tests were executed because the review instructions prohibit them; this conclusion is based on code and submitted-test/output inspection.
  • Observability, persistence, and writes: no runtime operation, transaction, persistence/EditLog, data-write, crash-recovery, or new observability surface is involved.
  • Performance: the added work is bounded analysis-time traversal and a linear signature-candidate scan; no material runtime hot-path cost or redundant distributed work was found.
  • Other review risks: ambiguous template recovery and vararg fallback were adversarially traced; reachable built-ins retain the correct final Decimal signatures. Existing inline threads were treated as duplicate fences, and no distinct unresolved candidate remains.

User focus: no additional review focus was provided.

Review completion: complete. The main scan, two normal coverage passes, and a separate risk-focused pass converged with NO_NEW_VALUABLE_FINDINGS; no inline comments are proposed.

@starocean999

Copy link
Copy Markdown
Contributor Author

run buildall

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-H: Total hot run time: 17065 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpch-tools
Tpch sf100 test result on commit 33925f824c1c431cfabd260ad23256ac8ffdfbc0, data reload: false

------ Round 1 ----------------------------------
============================================
q1	17621	3066	3022	3022
q2	2260	256	242	242
q3	10061	870	511	511
q4	4671	257	203	203
q5	7672	588	397	397
q6	134	122	94	94
q7	537	511	386	386
q8	9253	969	1005	969
q9	3505	2415	2402	2402
q10	6549	861	738	738
q11	393	201	179	179
q12	618	258	199	199
q13	18124	1520	1169	1169
q14	164	148	137	137
q15	q16	430	394	373	373
q17	1351	935	806	806
q18	3120	2251	2255	2251
q19	1108	905	769	769
q20	377	295	206	206
q21	4911	1784	1881	1784
q22	332	264	228	228
Total cold run time: 93191 ms
Total hot run time: 17065 ms

----- Round 2, with runtime_filter_mode=off -----
============================================
q1	3402	3360	3360	3360
q2	508	387	380	380
q3	2262	2385	2166	2166
q4	1190	1174	884	884
q5	2157	2141	2083	2083
q6	170	121	91	91
q7	1027	890	863	863
q8	1608	1399	1415	1399
q9	3148	3113	3118	3113
q10	1855	1784	1641	1641
q11	352	270	248	248
q12	453	426	351	351
q13	1501	1520	1144	1144
q14	172	166	164	164
q15	q16	395	389	360	360
q17	3606	3316	3296	3296
q18	4895	4438	4719	4438
q19	883	894	895	894
q20	1020	962	842	842
q21	3832	3095	3325	3095
q22	401	355	310	310
Total cold run time: 34837 ms
Total hot run time: 31122 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
TPC-DS: Total hot run time: 82440 ms
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/tpcds-tools
TPC-DS sf100 test result on commit 33925f824c1c431cfabd260ad23256ac8ffdfbc0, data reload: false

query5	4292	419	339	339
query6	400	138	129	129
query7	4915	415	236	236
query8	295	124	116	116
query9	8694	2895	2901	2895
query10	479	217	180	180
query11	5380	1021	938	938
query12	120	73	67	67
query13	1212	454	331	331
query14	6028	2196	2086	2086
query14_1	1989	1951	1974	1951
query15	175	119	124	119
query16	936	377	349	349
query17	811	470	361	361
query18	2329	327	240	240
query19	182	140	115	115
query20	72	71	74	71
query21	211	103	88	88
query22	5355	5365	5330	5330
query23	6640	6136	6196	6136
query23_1	6021	6213	6043	6043
query24	7282	1087	776	776
query24_1	768	754	767	754
query25	432	281	246	246
query26	1235	226	123	123
query27	2805	415	258	258
query28	4740	1488	1500	1488
query29	937	420	353	353
query30	254	159	128	128
query31	832	404	324	324
query32	132	76	87	76
query33	477	217	177	177
query34	1040	840	480	480
query35	413	385	353	353
query36	571	590	530	530
query37	119	77	69	69
query38	1027	847	816	816
query39	497	503	464	464
query39_1	453	456	466	456
query40	222	90	79	79
query41	55	52	54	52
query42	76	75	72	72
query43	240	240	208	208
query44	1027	540	551	540
query45	112	107	98	98
query46	762	831	517	517
query47	778	769	702	702
query48	294	302	228	228
query49	542	232	190	190
query50	741	258	198	198
query51	8286	8284	8250	8250
query52	68	66	71	66
query53	195	205	148	148
query54	276	173	168	168
query55	73	62	58	58
query56	225	168	172	168
query57	697	718	605	605
query58	228	175	156	156
query59	1193	1239	1075	1075
query60	254	190	168	168
query61	118	116	117	116
query62	366	202	178	178
query63	165	138	137	137
query64	2768	685	616	616
query65	1638	1607	1599	1599
query66	1872	260	222	222
query67	9739	9968	9799	9799
query68	2902	1239	795	795
query69	362	230	201	201
query70	706	606	610	606
query71	270	176	164	164
query72	2436	1757	1566	1566
query73	655	614	337	337
query74	1983	1214	1140	1140
query75	1183	1086	950	950
query76	2301	757	526	526
query77	259	251	212	212
query78	3871	3797	3141	3141
query79	2319	832	582	582
query80	1616	319	290	290
query81	535	152	133	133
query82	733	123	94	94
query83	288	210	193	193
query84	301	112	88	88
query85	800	357	335	335
query86	451	176	164	164
query87	1010	974	881	881
query88	2777	2103	2117	2103
query89	320	198	180	180
query90	2000	128	125	125
query91	134	116	98	98
query92	79	72	70	70
query93	1504	1067	681	681
query94	696	244	211	211
query95	530	262	226	226
query96	848	598	274	274
query97	1084	1068	1006	1006
query98	171	135	141	135
query99	449	349	316	316
Total cold run time: 178531 ms
Total hot run time: 82440 ms

@hello-stephen

Copy link
Copy Markdown
Contributor
ClickBench: Total hot run time: 14.63 s
machine: 'aliyun_ecs.c7a.8xlarge_32C64G'
scripts: https://github.com/apache/doris/tree/master/tools/clickbench-tools
ClickBench test result on commit 33925f824c1c431cfabd260ad23256ac8ffdfbc0, data reload: false

query1	0.00	0.01	0.00
query2	0.10	0.05	0.03
query3	0.26	0.11	0.11
query4	1.60	0.09	0.09
query5	0.18	0.15	0.16
query6	1.20	0.69	0.67
query7	0.03	0.01	0.00
query8	0.05	0.03	0.03
query9	0.29	0.22	0.21
query10	0.36	0.35	0.34
query11	0.17	0.12	0.12
query12	0.15	0.12	0.13
query13	0.32	0.31	0.31
query14	0.46	0.46	0.45
query15	0.36	0.34	0.35
query16	0.25	0.25	0.21
query17	0.69	0.69	0.69
query18	0.18	0.17	0.18
query19	1.16	1.20	1.08
query20	0.01	0.02	0.02
query21	15.47	0.15	0.12
query22	5.12	0.04	0.04
query23	16.09	0.25	0.10
query24	3.07	0.30	0.29
query25	0.13	0.04	0.04
query26	0.75	0.16	0.12
query27	0.11	0.03	0.03
query28	3.54	0.58	0.28
query29	12.45	3.20	2.55
query30	0.26	0.12	0.12
query31	2.76	0.38	0.18
query32	3.48	0.32	0.23
query33	1.38	1.40	1.47
query34	15.35	2.25	1.78
query35	1.78	1.71	1.73
query36	0.49	0.29	0.29
query37	0.06	0.04	0.04
query38	0.04	0.03	0.03
query39	0.03	0.02	0.03
query40	0.12	0.07	0.06
query41	0.09	0.03	0.02
query42	0.04	0.02	0.03
query43	0.03	0.02	0.03
Total cold run time: 90.46 s
Total hot run time: 14.63 s

@hello-stephen

Copy link
Copy Markdown
Contributor

FE Regression Coverage Report

Increment line coverage 73.66% (193/262) 🎉
Increment coverage report
Complete coverage report

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants