Skip to content

[fix](fe) Keep MAP key/value decimal slots independent in default deimal v3 precision promotion - #67154

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

[fix](fe) Keep MAP key/value decimal slots independent in default deimal v3 precision promotion#67154
starocean999 wants to merge 2 commits into
apache:masterfrom
starocean999:master_0527

Conversation

@starocean999

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #xxx

Related PR: #xxx

Problem Summary:
Nereids' default DECIMALV3 precision promotion
(ComputeSignatureHelper.defaultDecimalV3PrecisionPromotion) recursively collects every
DECIMALV3 found in the argument types, computes a single global "wider" type, and then
replaces ALL decimal slots with that one type via
TypeCoercionUtils.replaceDecimalV3WithTarget.

For MAP<K, V> where K and V are independent type variables with different
precision/scale, merging them into one wider type breaks correctness in two ways
(they share the same type-derivation path):

  1. MAP_KEYS (and other map consumers) silently turns legal keys into NULL.
    Repro: a MAP<DECIMAL(76,0), DECIMAL(76,18)> built from a 76-digit integral key
    and 0.125000000000000000. Widening the key slot to scale 18 overflows the big
    integral key, so MAP_KEYS returns [null] instead of the key.
  2. UNNEST(MAP) (rewritten to EXPLODE_MAP) fails type analysis. The promoted MAP
    argument type no longer matches the original input MAP type.

Fix: promote each decimal slot independently instead of merging them. Each slot
(MAP key/value, ARRAY item, scalar) keeps its own precision/scale derived from its
own argument type; the wider type is only used for NULL-typed slots and the
placeholder return type. Vararg slots keep the previous behavior to avoid changing
vararg decimal functions (e.g. field).

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.

…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.
@starocean999

Copy link
Copy Markdown
Contributor Author

/review

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