Skip to content

Refactor midpoint function and fix many logical errors, overflow and functional bugs#92102

Merged
nihalzp merged 17 commits intoClickHouse:masterfrom
nihalzp:fix-midpoint
Dec 17, 2025
Merged

Refactor midpoint function and fix many logical errors, overflow and functional bugs#92102
nihalzp merged 17 commits intoClickHouse:masterfrom
nihalzp:fix-midpoint

Conversation

@nihalzp
Copy link
Copy Markdown
Member

@nihalzp nihalzp commented Dec 13, 2025

Logical error:

SELECT midpoint(materialize(toNullable(1)), 11);

Wrong result for mixed numeric types:

SELECT midpoint(toUInt32(100), toDecimal32(200, 3));

Outputs 100.05 but should be 150.

Inconsistent rounding behavior:

SELECT midpoint(-4, -5); 
-- Returns -5 (rounds towards neg infinity)

SELECT midpoint(-10, -4, -2, -2); 
-- Returns -4 (rounds towards 0)

Overflow:

SELECT midpoint(toInt64('9223372036854775807'), toInt64('9223372036854775807'), toInt64('9223372036854775807'));

Returns 3074457345618258601, but should be 9223372036854775807.

Changelog category (leave one):

  • Critical Bug Fix (crash, data loss, RBAC) or LOGICAL_ERROR

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fix numerous logical errors, overflow and functional bugs in midpoint function. Closes #91816.

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@nihalzp nihalzp requested a review from Copilot December 13, 2025 21:01
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the midpoint function to fix multiple critical bugs including incorrect handling of nullable types, mixed numeric type conversions, inconsistent rounding behavior, and integer overflow issues. The fix implements proper nullable-aware semantics that ignore NULL values per row and uses wider integer types during summation to prevent overflow.

Key changes:

  • Reimplements nullable handling to correctly ignore NULL arguments per row instead of treating all rows as NULL
  • Fixes integer overflow by using wider types (Int128/UInt128/Int256/UInt256) during summation before division
  • Corrects rounding behavior for signed integers to consistently truncate toward zero

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
src/Functions/midpoint.h Complete refactor of midpoint logic: adds nullable-aware calculation paths, overflow-safe summation using wider types, and consistent truncation-toward-zero rounding for integers
src/Functions/avg2.cpp Simplifies avg2 (2-arg alias) by removing duplicate type resolution logic now handled by base class
tests/queries/0_stateless/03710_midpoint.sql Adds comprehensive test coverage for nullable types, overflow cases, mixed types, and edge cases
tests/queries/0_stateless/03710_midpoint.reference Expected results for new test cases
tests/queries/0_stateless/03710_midpoint_jit.sql Adds JIT compilation test coverage for both positive and negative number cases
tests/queries/0_stateless/03710_midpoint_jit.reference Expected results for JIT tests

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Dec 13, 2025

Workflow [PR], commit [ac5b006]

Summary:

job_name test_name status info comment
Integration tests (amd_asan, db disk, old analyzer, 4/6) failure
test_backup_restore_s3/test.py::test_backup_to_s3_different_credentials[data_file_name_from_first_file_name-native_multipart] FAIL cidb, issue
BuzzHouse (amd_debug) failure
Logical error: 'Inconsistent AST formatting in Function_tupleElement: the query: (STID: 1941-1bfa) FAIL cidb
BuzzHouse (amd_tsan) failure
Logical error: '(isConst() || isSparse() || isReplicated()) ? getDataType() == rhs.getDataType() : typeid(*this) == typeid(rhs)' (STID: 2508-3374) FAIL cidb
BuzzHouse (amd_ubsan) failure
Logical error: '(isConst() || isSparse() || isReplicated()) ? getDataType() == rhs.getDataType() : typeid(*this) == typeid(rhs)' (STID: 2508-2f8d) FAIL cidb

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Dec 13, 2025
@nihalzp nihalzp added v25.11-must-backport and removed pr-must-backport Pull request should be backported intentionally. Use this label with great care! labels Dec 13, 2025
@antonio2368 antonio2368 self-assigned this Dec 15, 2025
@nihalzp nihalzp added this pull request to the merge queue Dec 17, 2025
Merged via the queue into ClickHouse:master with commit 0564323 Dec 17, 2025
125 of 130 checks passed
@nihalzp nihalzp deleted the fix-midpoint branch December 17, 2025 11:56
@robot-ch-test-poll4 robot-ch-test-poll4 added the pr-synced-to-cloud The PR is synced to the cloud repo label Dec 17, 2025
robot-ch-test-poll4 added a commit that referenced this pull request Dec 17, 2025
Cherry pick #92102 to 25.11: Refactor `midpoint` function and fix many logical errors, overflow and functional bugs
robot-clickhouse added a commit that referenced this pull request Dec 17, 2025
…ogical errors, overflow and functional bugs
@robot-ch-test-poll3 robot-ch-test-poll3 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Dec 17, 2025
clickhouse-gh bot added a commit that referenced this pull request Dec 17, 2025
Backport #92102 to 25.11: Refactor `midpoint` function and fix many logical errors, overflow and functional bugs
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jan 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-critical-bugfix pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR pr-synced-to-cloud The PR is synced to the cloud repo v25.11-must-backport

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Fuzzer] midpoint produces empty Nullable result and fails with “Invalid number of rows in Chunk” on nullable literal input

6 participants