[fix](be) Preserve floating point values in CASE branch selection - #67896
Open
HappenLee wants to merge 2 commits into
Open
[fix](be) Preserve floating point values in CASE branch selection#67896HappenLee wants to merge 2 commits into
HappenLee wants to merge 2 commits into
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: N/A Problem Summary: Non-nullable FLOAT/DOUBLE CASE assembles results by multiplying branch values by zero or one and adding them to a zero-initialized result. An unselected Infinity or NaN therefore contaminates a finite selected result, and a selected negative zero loses its sign. For example, a three-row CASE selecting 1, an overflowing multiplication, and 2 returns NaN, Infinity, NaN instead. Use conditional stores for floating point columns so that only selected values are copied. This preserves their bits and lets Clang generate AVX2 masked loads/stores; a ternary source/result load can inhibit vectorization. Keep the other type paths unchanged. Add bitwise unit tests, SQL regression coverage and a benchmark that invokes the production result assembly function. ### Release note Fix incorrect FLOAT/DOUBLE CASE results caused by unselected non-finite branch values, and preserve selected negative zero. ### Check List (For Author) - Test: 12 ASAN unit tests; test_case_float_nonfinite and test_short_circuit_evaluation regression suites passed. The original code fails all 12 new unit tests and the new SQL regression. Golden output was generated and verified through short-circuit evaluation using the runner. ASAN BE build, clang-format 16, header hygiene and clang-tidy passed. RELEASE AVX2 benchmarks on Xeon Platinum 8457C cover 42 scenarios using identical input, fixed CPU and repeated before/after/after/before runs; median CPU time decreased by 5% to 39%. Inspected all four floating point and index-width specializations in the linked binaries for masked SIMD. - Behavior changed: Yes, return only the selected floating point branch value. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result
Request changes for one hot-path performance issue. The IEEE-754 correctness change itself is sound: ordinary CASE now copies the selected FLOAT/DOUBLE value without arithmetic contamination, while nullable/no-ELSE and short-circuit routes retain direct insertion/selector semantics.
Critical checkpoints
- Goal and proof: The implementation fixes unselected NaN/Infinity contamination and selected negative-zero loss. The bitwise typed unit matrix and ordered SQL cases cover FLOAT/DOUBLE, constants, non-finite values, signed zero, tails, both index widths, THEN/ELSE/no-ELSE, and both evaluation modes.
- Scope and clarity: The change is focused, but the old accumulator initialization should not remain on the new direct-store specialization; see the inline finding.
- Concurrency and lifecycle: No new threads, shared mutable state, locks, atomics, static initialization dependency, ownership cycle, or special teardown is introduced.
- Configuration and compatibility: No configuration, FE-BE field, function symbol, storage/serialization format, persistence, transaction, or rolling-upgrade contract changes.
- Parallel and conditional paths: Ordinary CASE builds a total compact branch index; nullable/no-ELSE uses
update_result_normal; short-circuit CASE partitions selectors and directly fills result positions. These paths remain semantically consistent. - Tests and expected results: Test discovery and private-access build flags are valid, and the checked golden output matches the query loop and row counts. The 4,099-row SQL comment does not force a physical batch boundary under the default batch size, but that is not a substantive gap because the kernel is stateless, the three-row SQL exposes the old bug, and direct tests cover 4,095/4,096/4,099 tails.
- Performance and memory: No new untracked owning buffer is introduced. MF-1 is the sole issue: a redundant O(rows) zero-write pass remains before total direct assignment. The benchmark inputs and stated AVX2 scope are otherwise structurally valid.
- Errors, data correctness, observability, and writes: No Status/error path, persisted state, data-write atomicity, visibility/version rule, or observability requirement is changed. No other substantiated issue was found.
- User focus: No additional user-provided focus was supplied.
This was a complete static review only; the review runner prohibits local builds and tests. Author-reported test/benchmark results were not independently executed. At submission time, BE UT, compile, and performance CI are still pending. Two full review rounds converged below the three-round cap, with all candidates adjudicated and the live base/head reverified at 0ff005b8d0bbaed15e435ebb92ce9b39502ed362.
Contributor
TPC-H: Total hot run time: 16989 ms |
Contributor
TPC-DS: Total hot run time: 83147 ms |
Contributor
ClickBench: Total hot run time: 14.76 s |
### What problem does this PR solve? Issue Number: N/A Related PR: apache#67896 Problem Summary: Date/time CASE result assembly uses a ternary assignment while FLOAT/DOUBLE uses conditional stores. Share the existing conditional store loop for DATE, DATETIME, DATEV2, DATETIMEV2, TIMESTAMP_NS and TIMESTAMPTZ to copy only the selected value and remove duplicated branch handling. Preserve the default-value initialization and the arithmetic accumulator for the remaining numeric types. Extend the existing typed selection tests to cover all eight types with both branch-index widths. ### Release note None ### Check List (For Author) - Test: Unit Test and Manual test - All 48 VCaseSelectionTest* tests pass with ASAN through run-be-ut.sh. - clang-format 16, header hygiene, git diff --check and clang-tidy pass. Header analysis uses the generated selection-test compiler flags. - ASAN BE compilation and linking pass; full packaging is blocked by an ASAN double-free in kuromoji_build_dict during OpenMP initialization. - Behavior changed: No; selected date/time values are preserved. - Does this need documentation: No
Contributor
Author
|
run buildall |
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
2 participants
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: N/A
Related PR: N/A
Problem Summary: A non-nullable FLOAT/DOUBLE CASE can return NaN for a finite selected branch when an unselected branch contains Infinity or NaN. For example, with ordinary CASE evaluation, selecting
1, an overflowing multiplication, and2over three rows returnsNaN, Infinity, NaNinstead of1, Infinity, 2. The same result assembly also loses the sign of selected negative zero.The result assembly multiplies each branch value by a zero/one mask and adds it to the result. IEEE-754 arithmetic makes
0 * Infinityand0 * NaNequal NaN. Use conditional stores to copy the selected floating point value without arithmetic. DATE, DATETIME, DATEV2, DATETIMEV2, TIMESTAMP_NS and TIMESTAMPTZ share this conditional-store loop, replacing their previous ternary assignments. This form allows AVX2 masked loads/stores, while a ternary assignment can become a conditional pointer load that inhibits vectorization.Retain the existing default-value initialization and the arithmetic accumulator for the other numeric types. Removing the redundant initialization for the direct-store types is deferred.
Add bitwise unit tests, SQL regression tests and a benchmark that directly calls the production result assembly function. The typed unit tests cover all eight types with both uint8_t and uint16_t branch indices.
Release note
Fix incorrect FLOAT/DOUBLE CASE results caused by unselected non-finite branch values, and preserve selected negative zero.
Check List (For Author)
Test
Validation of the date/time follow-up:
VCaseSelectionTest*tests pass via./run-be-ut.sh -j 48 --run --filter='VCaseSelectionTest*'with ASAN. Coverage includes all eight types, both index widths, constants, boundary values, vector tails and 255/257 branches.git diff --checkand clang-tidy pass. For clang-tidy, the header was explicitly mapped to the generated selection-test compiler flags to avoid failed automatic header inference../build.sh --be -j 48with ASAN compiled and linked the BE successfully. Full build/packaging did not pass: the laterkuromoji_build_dictprocess reported an ASAN double-free during OpenBLAS/OpenMP initialization (dlsym/_dlerror_run), before entering main. The separate standard ASAN unit-test build and execution passed.Earlier validation of the floating point fix (before the date/time follow-up):
test_case_float_nonfiniteandtest_short_circuit_evaluationpass. The original implementation fails the new SQL regression. Golden output was generated and independently verified through short-circuit evaluation using the regression runner. These SQL suites were not rerun for the date/time follow-up.-O3 -msse4.2 -mavx2on Xeon Platinum 8457C cover 42 scenarios. With identical input, a fixed CPU, five repetitions per run and before/after/after/before ordering, median CPU time decreases by approximately 5%–39% (18.5% geometric mean reduction). This measures floating point result assembly on this AVX2 machine, including result allocation; these figures do not measure the date/time follow-up.vmaskmovps/pd, replacing floating point multiply/add instructions without fast-math.Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)