[fix](join) preserve decimal precision for string comparison#65693
[fix](join) preserve decimal precision for string comparison#65693yx-keith wants to merge 6 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
1 similar comment
|
/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.
Requesting changes: the new lossless comparison mode has nine correctness gaps across SQL NULL semantics, BE memory safety, cache/clone identity, parser boundaries, predicate families, type-domain coverage, complex types, and COPY INTO translation.
Accepted issues:
- Conversion rejection is encoded as SQL NULL, breaking
<=>and typedNOT IN. - Rejected/source-NULL rows leave nested decimal storage uninitialized before comparison kernels read it.
VCastExpr::get_digest()aliases ordinary and lossless predicates in the condition cache.- Generic and format-v2 clone paths discard the mode and can prefilter an exact external-file match.
- Independent 1024 caps reject exact scientific values whose offsets cancel.
- Range/BETWEEN predicates use the changed common type but still round at full-width decimal boundaries.
LARGEINTis narrowed to a decimal type with only 38 integer digits although its domain needs 39.- Array/struct common-type recursion never enables lossless nested conversion.
- COPY INTO's translator override drops the flag.
Critical checkpoint conclusions:
- Goal and proof: the patch fixes the covered decimal/string join happy path, but does not accomplish the goal end to end because the cases above still miss matches or return false matches/NULL. The added tests prove only type shape, scalar parsing, and two happy-path counts.
- Focus and clarity: the change is conceptually focused, but a semantic expression flag necessarily spans planner reconstruction, FE-to-BE translation, BE cloning, digest identity, and alternate scan/load paths; several required paths were missed.
- Concurrency: the new state is per-expression/per-FunctionContext and the clone copies the context flag; no shared mutable-state, lock-order, or deadlock issue was found.
- Lifecycle: prepare/open/clone lifecycles were traced. No static-initialization or release issue was found, but expression cloning loses the semantic field as noted inline.
- Configuration: no new setting is added. Existing decimal256 and old/new coercion modes were checked; the boundary defects persist or move to the wider limit.
- Compatibility: the optional Thrift field is compatible with Doris's documented BE-then-FE upgrade order; a proposed reverse-order concern was dismissed with repository evidence.
- Parallel paths: ordering/BETWEEN, complex values, COPY INTO, and external-file localization are concrete missed paths.
- Conditional checks: the new exactness precheck is understandable, but its independent fixed caps are not equivalent to the parser's combined decimal-point calculation.
- Test coverage/results: expected counts in the new regression are consistent for its inputs, but negative coverage is not comprehensive. Missing cases include
<=>, typed NOT IN, range/BETWEEN, LARGEINT extrema, nested complex values, external files, COPY INTO, cache identity, vectorized NULL/failure rows, and long cancelling exponents. The regression also uses inlinetest/resultassertions and drops tables afterward instead of the repository's requested generated ordered result pattern/debug-preserving cleanup. - Observability: no new metrics/logging requirement was identified; these are deterministic result-correctness paths best covered by tests.
- Transactions, persistence, and data writes: no transaction/edit-log/storage-format mutation is introduced.
- FE/BE propagation: the ordinary Nereids translator and Thrift path carry the flag, but COPY INTO and BE clone/localization paths do not.
- Performance: no separate performance blocker was established. The extra string precheck is linear; condition-cache identity is a correctness defect, not merely a cache-efficiency concern.
- Other issues: BE nullable payload memory safety and live cache reuse are covered by the inline findings.
Validation was static-only as required by the review task; no build or test command was run. Live CI currently shows FE UT failed (the linked log requires authentication), two Clang Formatter jobs failed, and CheckStyle passed. The macOS BE UT failure is an environment setup/JDK mismatch; BE UT, compile, Cloud UT, and performance remain pending. No user focus area was supplied.
| right = castIfNotSameType(right, commonType.get()); | ||
| boolean losslessDecimalCast = comparisonPredicate instanceof EqualPredicate; | ||
| left = castComparisonOperand(left, commonType.get(), losslessDecimalCast); | ||
| right = castComparisonOperand(right, commonType.get(), losslessDecimalCast); |
There was a problem hiding this comment.
[correctness] Keep conversion rejection distinct from SQL NULL. NullSafeEqual is an EqualPredicate, so this analyzes (decimal_col <=> string_col) as NullSafeEqual(decimal_col, LosslessCast(string_col AS DECIMAL(38,0))). For a row (NULL, '100.4'), the cast marks the non-null string NULL because scale 0 cannot represent it, and eq_for_null then returns true because both null maps are set. The same sentinel makes CAST(100 AS DECIMAL(38,0)) NOT IN ('100.4') evaluate UNKNOWN instead of true. Please preserve source NULL separately from an inexact non-match (or use comparison-specific failure handling), and cover both <=> and typed NOT IN regressions.
| size_t next_offset = offsets[i]; | ||
| size_t string_size = next_offset - current_offset; | ||
| bool source_is_null = null_map != nullptr && null_map[i]; | ||
| null_map_to[i] = source_is_null |
There was a problem hiding this comment.
[correctness] Initialize the nested decimal slot on every NULL/failure path. nullable_to.resize() reaches PODArray::resize(), which only advances the end pointer. Here source_is_null || ... skips parsing for source NULLs, and from_string_lossless() returns before writing values_to[i] for rejected strings. Existing eq and eq_for_null paths evaluate the nested decimal vectors before applying their null maps, so these rows read indeterminate storage (UB/MSAN failure) even when the final value is NULL. Write a decimal default before the short circuit or on both failure paths, and add a vectorized test with source NULL and an inexact string.
| std::string _target_data_type_name; | ||
|
|
||
| DataTypePtr _cast_param_data_type; | ||
| bool _lossless_decimal_cast = false; |
There was a problem hiding this comment.
[correctness] Include this mode in VCastExpr::get_digest(). The current digest hashes the child/base expression and target type only, so ordinary and lossless casts collide despite different results. For a granule containing (d=100, s='100.4'), the implicit lossless predicate can cache the granule as all-false; a later explicit d = CAST(s AS DECIMAL(38,0)) has the same condition-cache key even though ordinary casting rounds and should match, so the cached bitmap suppresses the row. Hash _lossless_decimal_cast (or disable condition caching for this mode) and add a digest/cache distinction test.
| #endif | ||
| VCastExpr(const TExprNode& node) : VExpr(node) {} | ||
| VCastExpr(const TExprNode& node) | ||
| : VExpr(node), _lossless_decimal_cast(node.lossless_decimal_cast) {} |
There was a problem hiding this comment.
[correctness] Preserve the lossless mode in prefilter clones. VCastExpr::clone_node() rebuilds from clone_texpr_node(), which omits this field, while the format-v2 override recreates a cast from only its target type. For an external row (d=0, s='0e2147483648'), the original lossless cast treats zero as exact and accepts the parser's overflow status as value 0, but the localized ordinary cast returns NULL and filters the row before Scanner::_filter_output_block() can run the original residual. Copy the flag through both clone paths or refuse localization, and add this external-file false-negative regression.
| } | ||
| ++pos; | ||
| } | ||
| if (exponent_magnitude > max_relevant_exponent) { |
There was a problem hiding this comment.
[correctness] Combine the exponent and significand offset before applying a cap. For example, "0." + 1024 zeroes + "1e1025" is only 1032 bytes and equals exactly 1, which the existing StringParser accepts. This branch rejects every nonzero exponent above 1024 before adding the opposite base_decimal_point, so the new lossless equality turns that exact value into NULL and misses a match. Use overflow-safe signed arithmetic for the combined decimal-point position, and test large positive and negative offsets that cancel.
| } | ||
| left = castIfNotSameType(left, commonType.get()); | ||
| right = castIfNotSameType(right, commonType.get()); | ||
| boolean losslessDecimalCast = comparisonPredicate instanceof EqualPredicate; |
There was a problem hiding this comment.
[correctness] Do not use the scale-exhausted equality type for ordering. The new numeric/string common type also reaches <, <=, >, >=, and BETWEEN, but this flag is enabled only for equality. With DECIMAL(38,0), the common type remains scale 0, so ordinary parsing rounds '100.4' to 100: 100 >= '100.4' becomes true and 100 < '100.4' becomes false. Decimal256 just moves the same boundary to DECIMAL(76,0). Preserve enough information to order the fractional string without rounding, and add range/BETWEEN boundary tests in both coercion modes.
| } | ||
|
|
||
| private static DecimalV3Type getNumericStringComparisonType(NumericType numericType) { | ||
| DecimalV3Type decimalType = DecimalV3Type.forType(numericType); |
There was a problem hiding this comment.
[correctness] Preserve the full LARGEINT integer range here. DecimalV3Type.forType(LargeIntType) is DECIMAL(38,0), but signed LARGEINT extrema require 39 digits. This helper therefore chooses DECIMAL(38,0) with decimal256 off, or e.g. DECIMAL(44,6) with it on—still only 38 integer digits. The analyzed tree casts both the LARGEINT slot and string slot to that type, so a maximum LARGEINT and its identical text overflow to NULL instead of matching. Derive capacity from the actual 39-digit range (with a non-narrowing fallback when decimal256 is unavailable) and test both extrema under both settings.
|
|
||
| private static Expression castComparisonOperand( | ||
| Expression input, DataType targetType, boolean losslessDecimalCast) { | ||
| if (losslessDecimalCast && input.getDataType().isStringLikeType() |
There was a problem hiding this comment.
[correctness] Apply lossless coercion recursively for complex comparison types. Common-type derivation recurses, so ARRAY<DECIMAL(38,0)> = ARRAY<STRING> becomes an outer cast to ARRAY<DECIMAL(38,0)>; this scalar-only check sees neither an outer string nor an outer decimal and creates an ordinary cast. BE then rounds the nested '100.4' to 100, making [100] = ['100.4'] true. Struct-valued IN has the same gap. Detect nested numeric/string coercion and mark the containing cast (the BE wrapper propagates its context), or reject these shapes until supported; add array equality and struct-IN controls.
| // left child of cast is expression, right child of cast is target type | ||
| CastExpr castExpr = new CastExpr(cast.getDataType().toCatalogDataType(), | ||
| cast.child().accept(this, context), cast.nullable()); | ||
| cast.child().accept(this, context), cast.nullable(), cast.isLosslessDecimalCast()); |
There was a problem hiding this comment.
[correctness] Update the COPY INTO visitCast override as well. CopyIntoInfo.ExpressionToExpr analyzes file filters/column mappings with this coercion, but then reconstructs every cast with the three-argument CastExpr constructor, which defaults losslessDecimalCast to false. A numeric/string file filter therefore reaches BE as ordinary CAST and can round '100.4' to scale-zero 100. Forward both isLosslessDecimalCast() and the implicit/explicit bit there (or delegate to this implementation), and add a COPY INTO translation/end-to-end regression.
There was a problem hiding this comment.
Requesting changes. Nine correctness comments landed on the unchanged head while this review was in progress; I reread them and suppressed every overlap. This review adds two distinct issues below.
Review completeness: the third and final allowed convergence round produced the nested-column-pruning finding below. All currently known candidates have clear dispositions, but because a valuable candidate appeared at the three-round cap, this review is incomplete under the automated-review contract.
Critical checkpoint conclusions:
- Goal and proof: The direct scalar
=join examples are improved and their new counts are internally consistent, but the live threads show incomplete null-aware, range, LARGEINT, nested-type, parser, copy/clone, cache, and COPY INTO behavior. The additional inline findings show that reusable server-prepared point queries bypass the semantics and nested-column pruning later erases the marker. - Scope and focus: Marker propagation is localized in the direct path, but common-type selection changes non-equality comparisons and several parallel execution/translation/rewrite paths were not updated. The implementation is not yet focused enough to preserve existing semantics outside the target case.
- Concurrency: No new threads, shared mutable state, locks, or lock-order concerns are introduced.
- Lifecycle: Direct
VCastExpr::prepareandFunctionContext::clonesetup is sound. Existing live threads cover lost mode in BE/file-local copies, while the new pruning comment covers a later FE expression lifecycle; no static-initialization or ownership-cycle issue was found. - Configuration: No configuration item is added. Existing Decimal256/overflow-scale and nested-pruning settings affect behavior, and the tests do not cover the important on/off boundaries.
- Compatibility: The optional Thrift field is backward-readable, old-FE/new-BE safely defaults false, and the documented BE-first/FE-last rolling-upgrade order avoids the unsafe direction. No supported-upgrade blocker was retained.
- Parallel paths and conditions: Existing threads cover null-safe/IN behavior, ordering, LARGEINT, outer nested casts, COPY INTO, BE clones/digests, nullable storage, and exponent cancellation. The two comments below add cached prepared point-query reuse and nested-column-pruning reconstruction.
- Tests and expected results: Added scalar join counts look correct, but negative/end-to-end coverage is incomplete. No build was run because this review runner explicitly prohibits builds. CheckStyle and Cloud UT pass; both Clang Formatter checks fail, and a local non-mutating clang-format dry run confirms violations in changed C++ files. FE UT and BE UT are failing; compile and performance checks are still pending at review time.
- Observability: No new persistent operational state needs logs or metrics; existing query diagnostics are sufficient once the semantic gaps are fixed.
- Transactions, persistence, and writes: No EditLog, visible-version, delete-bitmap, transaction atomicity, or crash-recovery behavior is changed.
- FE-to-BE passage: Direct expression translation, Thrift serialization, BE construction, and function-context setup carry the flag. Existing threads and the new comments identify incomplete semantic copy/identity, rewrite, and alternate serialization paths.
- Performance and safety: The validator adds a linear lexical pass before parsing, but no independent performance, memory-tracking, or unchecked-Status blocker was substantiated. A live thread already covers the nested-column initialization safety issue.
- User focus: No additional focus points were supplied; the full PR was reviewed.
The regression also drops tables at suite end instead of preserving them for debugging, and the changed C++ files need repository formatting before merge.
| left = castIfNotSameType(left, commonType.get()); | ||
| right = castIfNotSameType(right, commonType.get()); | ||
| boolean losslessDecimalCast = comparisonPredicate instanceof EqualPredicate; | ||
| left = castComparisonOperand(left, commonType.get(), losslessDecimalCast); |
There was a problem hiding this comment.
[P1] Apply lossless coercion to cached point parameters
A cached server-prepared point query never reaches this coercion on later executions. For a row-store MOW table with a DECIMAL(38,0) key 100, execute WHERE k = ? once with VARCHAR 100 to cache the plan, then again with VARCHAR 100.4. ExecuteCommand reuses the context, PointQueryExecutor substitutes the raw string and serializes a DECIMAL_LITERAL, and BE's schema-scale decimal parser rounds it to key 100, returning a row that the uncached lossless predicate rejects. Validate replacement values losslessly before point-key serialization or invalidate/replan, and add this two-execution regression.
| public Cast withChildren(List<Expression> children) { | ||
| Preconditions.checkArgument(children.size() == 1); | ||
| return new Cast(children, targetType, isExplicitType); | ||
| return new Cast(children, targetType, isExplicitType, losslessDecimalCast); |
There was a problem hiding this comment.
[P1] Preserve lossless mode in nested-column pruning rewrites
This withChildren path now keeps the flag, but default nested-column pruning bypasses it. For k DECIMAL(38,0) = s.a with s STRUCT<a:STRING,...>, pruning rewrites the struct slot/ElementAt; SlotTypeReplacer.rewriteCast then rebuilds the marked scalar cast with new Cast(newChild, newType), whose constructor resets losslessDecimalCast to false. A row with k=100 and s.a='100.4' is consequently rounded and falsely matches. Preserve the original explicit/lossless attributes in that rewrite and add a pruning-enabled STRUCT/MAP/ARRAY field regression.
|
/review |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Automated review result: REQUEST_CHANGES
Review completed against c93579194bc8f2254a38b8a5d5afa23fd3d202d4. Three new, non-duplicate blocking issues remain: malformed overflowing exponents can become exact zero matches; external predicate translation can erase the exact-cast semantics before a pushed LIMIT; and the lossless wrapper silently bypasses enable_strict_cast. After the M2 proof example was corrected, all three final-round review tracks returned NO_NEW_VALUABLE_FINDINGS for the same bodies and anchors.
Critical checkpoint conclusions:
- Goal and proof: The patch's goal is exact DECIMAL/STRING equality without rounding. The normal Nereids-to-Thrift-to-BE path implements that goal for the basic join cases, and the added counts are correct, but M1-M3 show the proof is incomplete at lexical validation, external execution, and strict-mode boundaries.
- Scope and focus: The cross-layer flag changes are mostly focused and reuse existing cast machinery. The same-version reconstruction fixes cover normal/COPY translation, nested-slot replacement, folding, simplification, point-query recognition, BE cloning, and format-v2 localization. Generic external cast consumers were not included, producing M2.
- Concurrency and thread safety: No new threads, shared mutable state, lock ordering, or atomic synchronization are introduced. The FE expression attribute and BE function-context flag are per-plan/per-context state.
- Lifecycle and ownership: Cast, function-context, and format-v2 clone/prepare lifecycles were traced. Same-version clones preserve the flag, nullable storage is initialized on rejection paths, and no new ownership cycle, static initialization, or teardown issue remains.
- Configuration: No new configuration is added. Existing
decimal_overflow_scaleinfluences the comparison type as designed, but the existingenable_strict_castcontract is bypassed by the new branch (M3). - Compatibility: Optional Thrift field 43 defaults safely on a new BE, and the documented BE-then-FE rolling order does not require new-FE/old-BE execution. Independent older Remote Doris endpoints remain a supported external surface and can apply pre-fix coercion after the cast is stripped, which is part of M2.
- Parallel paths and conditions: Normal/COPY translators, runtime filters, predicate inference, partition evaluation, fresh point queries, prepared-query context, format-v2 residuals, ORC/Parquet pruning, and cache digests were checked. Known prepared, NULL-semantics, range, complex-type, and nested-pruning cases were deduplicated against existing threads. The distinct missed parallel path is external SQL/connector translation plus remote LIMIT.
- Conditional logic: The
EqualTo-only marking and exactness arithmetic were traced across DecimalV2/V3 and decimal128/256 boundaries. The exponent-overflow early return is not grammar-safe (M1), and lossless-before-strict precedence changes error behavior (M3). - Test coverage: Unit and regression tests cover representative exact/inexact strings and two join counts, but they do not cover malformed suffixes after exponent overflow, strict-mode expected errors, or external predicate plus LIMIT. The trailing table drops also depart from the repository's preferred test-cleanup form, although a failing assertion exits before them and preserves failure state; this was treated as nonblocking hygiene.
- Test results: No local build or repository test command was run because the review contract explicitly forbids builds. A standalone 500,000-case exact-rational property check found no bounded-valid mismatch beyond the malformed huge-exponent-zero family. Current CI has CheckStyle, both Clang Formatter jobs, Cloud UT, license, secrets, and dependency checks passing; macOS BE UT failed because the runner selected JDK 25 instead of required JDK 17, while BE UT, FE UT, compile, and performance jobs remain pending.
- Observability: This change adds expression semantics rather than a new long-running or distributed state machine; existing query errors/explain/debug paths are sufficient once correctness is fixed. No new metric or log is required.
- Transactions, persistence, and data writes: No production transaction, EditLog, persisted metadata, storage format, or write atomicity path is changed.
- FE-BE propagation: Same-version FE-to-BE transport, legacy
CastExprclone/Gson state,VCastExpr,FunctionContext, format-v2 clones, equality/hash, and condition-cache digesting preserve the bit. M2 is specifically outside that path: external translators unwrap the semantic cast. - Performance: The validator adds a linear scan and the selected decimal type can widen join keys, but no independent safe optimization was established: narrowing the type under the current nullable-cast encoding would turn more unequal finite fractions from false into NULL. No additional blocking CPU, allocation, or memory-lifetime issue was found.
- Other correctness: M1 is distinct from the existing valid cancelling-exponent thread because it accepts malformed input rather than rejecting valid input. M3 is distinct from the existing SQL-NULL thread because it changes query failure into success for ordinary equality.
User-provided focus: no additional focus points were supplied; the complete PR was reviewed.
| // left child of cast is expression, right child of cast is target type | ||
| CastExpr castExpr = new CastExpr(cast.getDataType().toCatalogDataType(), | ||
| cast.child().accept(this, context), cast.nullable()); | ||
| cast.child().accept(this, context), cast.nullable(), cast.isLosslessDecimalCast()); |
There was a problem hiding this comment.
[P1] Keep this semantic cast out of generic remote pushdown — This bit survives BE translation here, but both ExprToExternalSqlVisitor and ExprToConnectorExpressionConverter erase every CastExpr. Remote Doris/JDBC can therefore execute a lossy d = s predicate and also receive the scan LIMIT. For remote rows (9007199254740992,'9007199254740992.0000004') before the exact (9007199254740992,'9007199254740992'), legacy DOUBLE coercion loses the fraction and the newer pre-fix fixed-numeric path rounds it at scale six, so WHERE d = s LIMIT 1 can return only the false positive; the local lossless residual drops it and cannot recover the exact row. Please preserve an equivalent exact predicate only for connectors that support it, or leave this as a local residual and suppress remote LIMIT pushdown.
| if constexpr (type_allow_cast_to_decimal<FromDataType>) { | ||
| if (context->enable_strict_mode()) { | ||
| if constexpr (std::is_same_v<FromDataType, DataTypeString>) { | ||
| if (context->enable_lossless_decimal_cast()) { |
There was a problem hiding this comment.
[P1] Preserve strict-cast failures in lossless comparisons — Because this branch runs before enable_strict_mode(), a marked comparison under enable_strict_cast=true no longer uses from_string_strict_mode_batch: malformed text such as 100abc and decimal overflow become NULL non-matches instead of failing the query. The new regression explicitly demonstrates that behavior, but it contradicts the existing session contract (“Use strict mode for cast”) and changes failure into silent success. Please keep strict lexical/overflow errors while separately rejecting well-formed but inexact comparison values, and add expected-error coverage.
| return false; | ||
| } | ||
| if (exponent_magnitude > (std::numeric_limits<int64_t>::max() - (ch - '0')) / 10) { | ||
| return first_non_zero == 0; |
There was a problem hiding this comment.
[P1] Finish validating the exponent before accepting zero — For 0e9223372036854775808x, this returns true as soon as the exponent overflows because the significand is zero, without ever validating the trailing x. StringParser::string_to_decimal also stops at its smaller exponent cap, returns PARSE_OVERFLOW with value 0, and from_string_lossless accepts that status, so this malformed VARCHAR becomes a non-NULL zero and incorrectly matches DECIMAL zero. Please keep scanning the exponent grammar after magnitude overflow and only accept zero once the whole token is valid; this is the opposite direction from the existing cancelling-exponent thread.
TPC-H: Total hot run time: 29683 ms |
TPC-DS: Total hot run time: 177996 ms |
ClickBench: Total hot run time: 25.16 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
|
/review |
|
run buildall |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
There was a problem hiding this comment.
Automated review result: COMMENT — no new non-duplicate inline findings
Reviewed head 81c97d5c9db1dafcef62c6918908a9779c998d3b against the authoritative 26-file patch and all 14 existing inline threads. After a main scan, three independent reviews, deduplication, and convergence, no additional inline issue survived. This is not an approval: the remaining blocking correctness concerns are already covered by existing threads, notably conversion rejection leaking SQL NULL semantics, nested complex comparisons, prepared point-query reuse, remote predicate/LIMIT pushdown, strict-cast error suppression, and malformed huge exponents. Reposting them would duplicate the current review context.
Current-head progress: the patch now narrows the feature to ordinary EqualTo, initializes rejected decimal payloads, preserves the marker through FE/BE and format-v2 clones, includes it in the scanner digest, carries it through COPY and nested-slot rewrites, skips unsafe fold/simplify and the direct marked point-query shortcut, and fixes cancelling exponents. Those changes address the corresponding earlier comments.
Critical checkpoint conclusions:
- Goal and proof: The scalar decimal/string equality path now prevents rounded false matches and has BE helper, FE coercion/Thrift, and hash-join regression coverage. The goal is not yet complete because the existing correctness threads above remain unresolved, and negative coverage does not exercise those paths.
- Scope and clarity: The implementation is cross-layer but focused on one semantic marker and the reconstruction/serialization paths that must preserve it. No unrelated production behavior was found.
- Concurrency: No new shared mutable state, thread entry, lock, atomic, or lock-order dependency is introduced. The mode is immutable per expression and copied into thread-local
FunctionContextclones. - Lifecycle: Expression/function-context preparation and clone lifetimes were traced through ordinary execution and format-v2 localization. No new static-initialization, ownership cycle, cleanup, or shutdown boundary is added; changed
Statusresults are propagated. - Configuration: No configuration item is added. Existing
enable_decimal256,decimal_overflow_scale, strict-cast, and coercion settings affect planning/execution as expected, subject to the existing strict-mode comment. - Compatibility: Optional Thrift field 43 defaults false for older senders. A new FE has no old-BE field gate, but the supported BE-first cluster-upgrade sequence excludes that execution state; no storage/function-symbol compatibility change is introduced.
- Parallel paths and conditions: Generic/COPY translation, expression and slot reconstruction, folding, simplification, runtime-filter translation, point-query eligibility, format-v2 localization, residual filtering, and cache identity were checked. Still-live remote, complex, prepared-parameter, NULL, and strict/error variants are existing-thread duplicates. The
EqualToeligibility condition is clear but does not by itself resolve those existing families. - Tests/results: Added tests cover the core helper, FE coercion/marker/Thrift behavior, both coercion settings, and one end-to-end hash join. They do not comprehensively cover existing negative/error/NULL/remote/point/complex/format-v2 cases. No local build or test was run because the review contract prohibits it. At submission time, style checks pass; COMPILE, performance, and macOS BE UT are failing, while Linux BE UT and FE UT are pending.
- Observability: This is a local expression semantic mode; existing query error/profile facilities are sufficient, and no new log or metric is warranted.
- Transactions, persistence, and writes: None are involved; there is no EditLog, failover, storage-format, atomic-write, or crash-recovery boundary.
- FE/BE propagation: The new value is serialized by the FE, read by BE, and preserved across the changed translators,
CastExpr/VCastExpr, function-context, and format-v2 clone paths. Remaining erasure/alternate-execution cases are already covered by existing threads. - Performance and other risks: Lossless parsing adds one bounded lexical validation pass. Maximum-width decimal coercion is not a new default-path regression—the base default already selected the same DECIMAL128/256 common type—and narrowing locally would change projected/negated equality semantics. No new allocation, memory-safety, CPU, or observability defect survived review.
Validation was static only, per the review-runner contract.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
81c97d5 to
4b47820
Compare
|
run buildall |
|
/review |
There was a problem hiding this comment.
Automated review result: REQUEST_CHANGES
Reviewed head 18ad9ebad643e1e9a3c1549788a4f9d0afb2cf75. Three new, non-duplicate P1 issues remain: format-v2 localization can hide strict-cast errors after selected-row compaction; legacy catalog predicate converters can lexically prune numerically equivalent strings; and the legacy Trino scan can push LIMIT before the marked comparison's local residual. Three independent round-2 full-PR reviews returned NO_NEW_VALUABLE_FINDINGS for this exact comment set after all candidates and existing threads were deduplicated.
Critical checkpoint conclusions:
- Goal and proof: The primary scalar DECIMAL/STRING equality path now rejects rounded false matches and preserves malformed/overflow failures in strict mode. The parser, FE coercion, Thrift, and core join tests support that path, but the three inline issues show the goal is not complete across file-local and legacy connector execution.
- Scope and clarity: The marker-based implementation is conceptually focused, but its semantics necessarily span FE rewrites, FE-to-BE transport, BE clones, file-local expressions, and every predicate/LIMIT pushdown path. The direct path is clear; the missed parallel paths are blocking.
- Concurrency and thread safety: No new thread entry, shared mutable state, lock, atomic, or lock-order dependency is introduced. The flag is immutable per expression and copied into per-query
FunctionContextclones. - Lifecycle and ownership: Cast construction, prepare/open/close, deep clone, format-v2 localization, connector filter planning, and residual evaluation were traced. No static-initialization, ownership-cycle, or teardown defect was found; M-BE-001 is the selected-row execution-contract loss during localization.
- Configuration: No configuration item is added. Existing strict-cast, Decimal256, overflow-scale, and coercion-mode settings were checked; the accepted issues do not depend on dynamic configuration races.
- Compatibility: Optional Thrift field 43 defaults false and is compatible with Doris's supported BE-then-FE rolling-upgrade order. No storage-format or function-symbol change is introduced.
- Parallel paths and conditions: Normal/COPY translation, nested-slot replacement, folding, simplification, direct point-query eligibility,
VCastExpr/FunctionContextclones, generic plugin scans, Remote Doris, condition-cache identity, Hudi/Paimon hybrid readers, and legacy Paimon/Iceberg/LakeSoul/HMS/Trino paths were checked. The three inline comments are the remaining distinct gaps; NULL-safe/IN, prepared point-query, ordering, LARGEINT, complex-type, and earlier pushdown concerns were suppressed as existing-thread duplicates. - Tests and expected results: The new parser unit test is registered, FE coercion/Thrift tests cover the marker, and the join regression's counts and strict failures are consistent. Coverage is missing for the three accepted paths, as requested inline. The regression's inline fixed results and trailing table drops also depart from the repository's preferred generated-result/debug-preserving pattern, but that is non-blocking hygiene.
- Observability: This is deterministic expression behavior with existing query errors, explain output, and scanner profiles; no additional log or metric is required.
- Transactions, persistence, and writes: No EditLog, transaction, visible-version, persisted metadata, storage write, atomicity, or crash-recovery behavior is changed.
- FE-BE propagation: The marker survives current Nereids reconstruction, legacy
CastExpr, ordinary and COPY translation, Thrift,VCastExpr,FunctionContext, digesting, and the value-mode portion of format-v2 localization. The accepted defects occur after that transport in selected-row scheduling and separate connector implementations. - Performance and safety: The lexical validator and expression marker scans are linear, localized condition caching is conservatively disabled, and no independent CPU, allocation, nullable-storage, unchecked-Status, or memory-lifetime blocker remains. M-BE-001 is an error-preservation issue, not a performance optimization request.
- Other issues: The literal pre-coercion lead was dismissed because precision/scale validation precedes decimal scale adjustment. Exhaustive/random parser differential checks found no remaining exact-token mismatch. No additional user focus was supplied; the full PR was reviewed.
Validation was static-only because the review contract prohibits builds and repository test commands. Live style checks pass. The macOS BE UT failure is an unrelated runner JDK 25 versus required JDK 17 setup failure; the main compile and unit-test jobs were still pending at the final audit.
| Status clone_node(VExprSPtr* cloned_expr) const override { | ||
| DORIS_CHECK(cloned_expr != nullptr); | ||
| *cloned_expr = Cast::create_shared(_data_type); | ||
| *cloned_expr = Cast::create_shared(_data_type, _lossless_decimal_cast); |
There was a problem hiding this comment.
[P1] Preserve the selected-row safety of this localized cast — The source VCastExpr always reports is_safe_to_execute_on_selected_rows() == false, but format::Cast inherits the deterministic-child default. Hudi/Paimon native readers replace the source with this class, so Parquet can run an earlier single-column predicate on the full batch, compact it, and evaluate a strict marked comparison only on survivors. For id=1,s='bad' plus WHERE id > 1 AND d = s, that suppresses the strict parse error that the original conjunct would raise. Please override the safety method to return false (or preserve it during cloning) and add the selected-row differential test.
| } else if (expr instanceof LiteralExpr) { | ||
| return convertLiteral((LiteralExpr) expr); | ||
| } else if (expr instanceof CastExpr) { | ||
| if (((CastExpr) expr).isLosslessDecimalCast()) { |
There was a problem hiding this comment.
[P1] Apply this guard to the catalog-specific predicate converters — Paimon, Iceberg, LakeSoul, and HMS do not call this converter; their legacy converters unwrap every CastExpr without checking the marker. For a Paimon VARCHAR s, WHERE s = 100.0 is analyzed as a lossless decimal comparison, but PaimonPredicateConverter pushes raw string equality to "100.0". That prunes s='1e2' or s='100.00', both of which the local lossless cast would match, and the residual cannot recover them. Please reject marked casts before each specialized file/partition/reader pushdown and add a noncanonical-string connector regression.
| return castExprs.stream().anyMatch(CastExpr::isLosslessDecimalCast); | ||
| } | ||
|
|
||
| public static boolean canPushDownLimit(List<Expr> conjuncts) { |
There was a problem hiding this comment.
[P1] Use this LIMIT guard in the legacy Trino scan too — TrinoConnectorPredicateConverter cannot convert d = LosslessCast(s) when both sides are slots, so it leaves TupleDomain.all() and Doris retains the comparison as a local residual. TrinoConnectorScanNode.applyPushDown, however, still calls connectorMetadata.applyLimit whenever the scan has a limit. With (100,'100.4') before (100,'100') and WHERE d = s LIMIT 1, Trino can return only the first row; the residual rejects it and the exact row is lost. Please gate Trino applyLimit with this helper and add a scan-level regression.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29920 ms |
TPC-DS: Total hot run time: 178267 ms |
ClickBench: Total hot run time: 25.28 s |
2 participants
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)