[BUG](exec) fix coalesce function output null#63092
[BUG](exec) fix coalesce function output null#63092qzsee wants to merge 1 commit intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
/review |
There was a problem hiding this comment.
The implementation change is small and directly addresses the reported COALESCE value-pollution path, but this PR currently has no regression or unit test for the bug it fixes. Because this is BE expression correctness code, Doris review standards require a targeted test before merge.
Critical checkpoint conclusions:
- Goal/test: The code appears to accomplish the stated goal for scalar non-string COALESCE results by avoiding arithmetic with null-position payloads, but no test proves the reported nullable DOUBLE/NaN case or prevents regression.
- Scope: The code change is focused and minimal.
- Concurrency/lifecycle: Not applicable; this is per-block expression evaluation with no new shared state or lifecycle management.
- Configuration/compatibility/protocol: Not applicable; no config, persisted format, or FE-BE protocol changes.
- Parallel paths: String/array/map/struct/JSON and bitmap paths already use guarded insertion/assignment; the changed scalar path is the relevant divergent path.
- Conditions/error handling: No new error path; the new guard is the necessary data-correctness condition.
- Tests/results: Missing targeted regression/unit coverage and no expected result update was included.
- Observability/transactions/data writes: Not applicable.
- Performance: The new branch is in a hot scalar loop but is justified by correctness; no obvious blocking performance issue found.
Focus points: No additional user-provided review focus was supplied.
| column_raw_data[row] * | ||
| typename ColumnType::value_type(!(null_map_data[row] | filled_flag[row])); | ||
| if (!(null_map_data[row] | filled_flag[row])) { | ||
| result_raw_data[row] = column_raw_data[row]; |
There was a problem hiding this comment.
This bug fix needs a targeted regression test. The PR description describes a concrete data-correctness failure where COALESCE(nullable_double, 0) can return NULL after a null-position payload contains NaN, but the PR only changes BE code and does not add a case under the existing conditional function/coalesce tests (or a BE unit test) that fails before this change and passes after it. Please add coverage for the nullable floating-point path so this does not regress.
| result_raw_data[row] += | ||
| column_raw_data[row] * | ||
| typename ColumnType::value_type(!(null_map_data[row] | filled_flag[row])); | ||
| if (!(null_map_data[row] | filled_flag[row])) { |
There was a problem hiding this comment.
add test please and also check the document
What problem does this PR solve?
Issue Number: close #xxx
Example: COALESCE(same_department_income_amount, 0) ==> outputs NULL (where same_department_income_amount is of type double).
When assigning the value to the result column in the computation, the assignment is done unconditionally (forced), as in:
result_raw_data[row] += column_raw_data[row] * typename ColumnType::value_type(!(null_map_data[row] | filled_flag[row]));If the argument column column_raw_data's null_map[row] is 1, then the value stored in column_raw_data[row] is garbage data. This garbage may contain values such as NaN. If a preceding argument of COALESCE happens to be assigned NaN, then during subsequent assignments we run into cases like:
0 * NaN = NaN
num + NaN = NaN
so the assigned result also becomes NaN, which causes value pollution.
By rights the final output should also be NaN, but what is actually returned is NULL. The reason is that during result serialization/output, NaN values are emitted as NULL.
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)