Skip to content

[BUG](exec) fix coalesce function output null#63092

Open
qzsee wants to merge 1 commit intoapache:masterfrom
qzsee:fix-coalesce
Open

[BUG](exec) fix coalesce function output null#63092
qzsee wants to merge 1 commit intoapache:masterfrom
qzsee:fix-coalesce

Conversation

@qzsee
Copy link
Copy Markdown
Contributor

@qzsee qzsee commented May 8, 2026

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.

tatus DataTypeNumberSerDe<T>::_write_column_to_mysql(const IColumn& column,
                                                      MysqlRowBuffer<is_binary_format>& result,
                                                      int row_idx, bool col_const,
                                                      const FormatOptions& options) const {
    //...
    else if constexpr (std::is_same_v<T, float>) {
        if (std::isnan(data[col_index])) {
            // Handle NaN for float, we should push null value
            buf_ret = result.push_null();
        } else {
            buf_ret = result.push_float(data[col_index]);
        }
    } 
  //...
}

Related PR: #xxx

Problem Summary:

Release note

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?

@HappenLee HappenLee changed the title [BUG] fix coalesce function output null [BUG](exec) fix coalesce function output null May 8, 2026
@zclllyybb
Copy link
Copy Markdown
Contributor

/review

Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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];
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.

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])) {
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.

add test please and also check the document

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.

4 participants