Repository navigation
Conversation
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: With check_overflow_for_decimal=true, scalar Decimal addition raises Arithmetic overflow, but query-side SUM/AVG still used unchecked += and silently wrapped. Inject RuntimeState overflow policy into query aggregate functions and use checked add/sub/merge (and AVG finalize scale multiply) for DecimalV3; storage sum_load/sum_reader keep wraparound. ### Release note DecimalV3 SUM/AVG now honor check_overflow_for_decimal and report Arithmetic overflow instead of wrapping when the flag is enabled. ### Check List (For Author) - Test: Unit Test / Regression test - BE UT AggDecimalOverflowTest; regression test_decimalv3_sum_avg_overflow - Behavior changed: Yes — overflowing DecimalV3 SUM/AVG fail when check_overflow_for_decimal=true (default); set false to keep old wrap - Does this need documentation: No
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: Query-side SUM only used int128 add_overflow. For Decimal(38,0), 10^38-1+1 still fits in int128 (~1.7e38) so no error was raised and a dirty out-of-precision value was returned. Align with scalar Decimal arithmetic by also rejecting results beyond get_max_digits_number(max_precision()) in SUM add/sub and AVG add/sub/finalize. ### Release note DecimalV3 SUM/AVG with check_overflow_for_decimal=true now fail when the result exceeds Decimal precision, not only when native integer addition overflows. ### Check List (For Author) - Test: Unit Test - AggDecimalOverflowTest already covers max+1 precision overflow for SUM - Behavior changed: Yes — overflowing past Decimal precision now errors when check is on - Does this need documentation: No Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
shiliang
changed the title
Author
|
run buildall |
### What problem does this PR solve? Issue Number: close apache#68123 Related PR: apache#69595 Problem Summary: BE UT failed to compile because EXPECT_NE on Decimal128V3 is incompatible with C++20/gtest (operator== returns auto, not bool). Compare .value instead. ### Release note None ### Check List (For Author) - Test: No need to test (test compile fix only) - Behavior changed: No - Does this need documentation: No Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
run beut |
Contributor
TPC-H: Total hot run time: 28868 ms |
Contributor
TPC-DS: Total hot run time: 151737 ms |
Contributor
ClickBench: Total hot run time: 24.11 s |
Co-authored-by: Cursor <cursoragent@cursor.com> # Conflicts: # be/src/exprs/aggregate/aggregate_function_avg.h # be/src/exprs/aggregate/aggregate_function_sum.h
Contributor
TPC-H: Total hot run time: 28870 ms |
### What problem does this PR solve? Issue Number: close apache#68123 Related PR: apache#69595 Problem Summary: P0 failed with Missing outputFile for test_decimalv3_sum_avg_overflow because the suite uses qt_/order_qt_ tags without a committed .out. ### Release note None ### Check List (For Author) - Test: No need to test (add expected .out for existing suite) - Behavior changed: No - Does this need documentation: No Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
run p0 |
Contributor
TPC-H: Total hot run time: 28988 ms |
Contributor
TPC-DS: Total hot run time: 151914 ms |
Contributor
ClickBench: Total hot run time: 24.43 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: close apache#68123 Related PR: apache#69595 Problem Summary: Formatting-only: fold add_sum/sub_sum args to clang-format-16 style; sort arena.h include; put max_int128 init on one line. No logic change. ### Release note None ### Check List (For Author) - Test: No need to test (format only) - Behavior changed: No - Does this need documentation: No Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 28769 ms |
Contributor
TPC-DS: Total hot run time: 152756 ms |
Contributor
ClickBench: Total hot run time: 24.42 s |
### What problem does this PR solve? Issue Number: close apache#68123 Related PR: apache#69595 Problem Summary: P0 failed on sum_wrap because IS NOT NULL returns boolean true, not integer 1. Update the .out expectation. ### Release note None ### Check List (For Author) - Test: No need to test (.out expectation fix from CI) - Behavior changed: No - Does this need documentation: No Co-authored-by: Cursor <cursoragent@cursor.com>
Author
|
run p0 |
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?
Issue Number: close #68123
Related PR: None
Problem Summary:
With
check_overflow_for_decimal=true(default), scalar Decimal arithmetic already reports[E-255] Arithmetic overflow, but query-sideSUM/AVGstill used unchecked+=and could return a silently wrong value.Reproduction (from #68123), two rows of
DECIMAL(38,0)holding10^38 - 1:a + a/a * 2SUM(a)AVG(a)There are two silent failure regimes for
SUM:10^38 - 1): result can look positive and plausible (e.g. about one tenth of the truth), with nothing in the log saying it is wrong.Root cause in BE:
AggFnEvaluator, but never injectedRuntimeState::check_overflow_for_decimal().AggregateFunctionSum/Avgaccumulated with unchecked add; even after addingadd_overflow, a value such as(10^38-1) + 1still fits in int128 (~1.7e38) and was accepted unless we also check Decimal precision (get_max_digits_number(max_precision())), same as scalarbinary_arithmetic.How it was fixed:
IAggregateFunction::set_check_overflow_for_decimal();AggFnEvaluator::preparesets it fromRuntimeState. Wrapper aggregations (null / distinct / sort / foreach / map / agg_state combine|merge|union) forward the flag to the nested function.SUM/AVG: on the checked path, usecommon::add_overflow/sub_overflow(and AVG finalizemul_overflowforsum * scale_multiplier), then reject results outside Decimal precision.sum_load/sum_readerfor AGGREGATE KEY import / compaction / read merge) do not go throughAggFnEvaluatorand keep historical wraparound — changing that would alter persisted pre-aggregate semantics and is out of scope for this PR (same limitation called out in [Bug] SUM() over DECIMAL silently returns a wrong value on overflow, while check_overflow_for_decimal errors on the same overflow in binary arithmetic #68123 for on-disk AGGREGATE KEY).AggDecimalOverflowTest; regressiontest_decimalv3_sum_avg_overflow(plain / group by / window / distinct / nullable;enable_decimal256=truestill succeeds for the same inputs).Release note
When
check_overflow_for_decimalis enabled (default), DecimalV3 query-sideSUM/AVG(including window andSUM(DISTINCT)/AVG(DISTINCT)) now raiseArithmetic overflowinstead of returning a wrapped or out-of-precision value. Setcheck_overflow_for_decimal=falseto keep the old wraparound behavior. AGGREGATE KEY storage pre-aggregation (sum_load/sum_reader) is unchanged.Check List (For Author)
Test
regression-test/suites/datatype_p0/decimalv3/test_decimalv3_sum_avg_overflow.groovy)be/test/exprs/aggregate/agg_decimal_overflow_test.cpp)10^38-1+1); check-off wrap; window / distinct / nullable;enable_decimal256=truepathBehavior changed:
SUM/AVGon overflowing DecimalV3 silently returned wrong values whilea+a/a*2already errored under the same session flag.SUM/AVGhonorcheck_overflow_for_decimaland error like scalar Decimal ops; flagfalserestores wraparound.true) makes overflowing aggregates fail fast instead of returning plausible wrong numbers. Workloads that relied on silent wrap must setcheck_overflow_for_decimal=falseor useenable_decimal256=truefor a wider intermediate type. Storage AGGREGATE KEY SUM columns still wrap on write as before.Does this need documentation?