Skip to content

Might fix monotonicity of the cast function for big integer types.#80783

Merged
yariks5s merged 2 commits intoClickHouse:masterfrom
zoomxi:fix_monotonicity_of_cast_func_for_big_int
Jun 10, 2025
Merged

Might fix monotonicity of the cast function for big integer types.#80783
yariks5s merged 2 commits intoClickHouse:masterfrom
zoomxi:fix_monotonicity_of_cast_func_for_big_int

Conversation

@zoomxi
Copy link
Copy Markdown
Contributor

@zoomxi zoomxi commented May 25, 2025

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 to CHANGELOG.md):

This PR might close #80742.

When casting from a big integer type, getMonotonicityForRange(UInt128, field, field) appears to be monotonic if field is null. However, if field's type is UInt128, the function seems to lose monotonicity, likely because only native integer types are currently supported.

To address this, the PR adds a check in ToNumberMonotonicity to verify whether the type is a big integer when field is null, ensuring correct monotonicity behavior.

@yariks5s yariks5s self-assigned this May 26, 2025
@yariks5s yariks5s added the can be tested Allows running workflows for external contributors label May 26, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented May 26, 2025

Workflow [PR], commit [f56a808]

@clickhouse-gh clickhouse-gh bot added pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! pr-must-backport-cloud labels May 26, 2025
@zoomxi zoomxi force-pushed the fix_monotonicity_of_cast_func_for_big_int branch from fc8e32a to 15fe4d0 Compare May 27, 2025 02:23
@zoomxi
Copy link
Copy Markdown
Contributor Author

zoomxi commented May 27, 2025

Performance Comparison (amd_release,master_head,3/3)

Performance Comparison (amd_release,master_head,2/3)

  • Job killed, exit code [1]

@yariks5s Could you please take a look? Thanks

@zoomxi zoomxi force-pushed the fix_monotonicity_of_cast_func_for_big_int branch from 15fe4d0 to 99378fe Compare May 29, 2025 01:27
@zoomxi
Copy link
Copy Markdown
Contributor Author

zoomxi commented May 29, 2025

@yariks5s I've rebased onto master, and all checks are passing. Please review this PR when you have a moment. Thanks.

@zoomxi
Copy link
Copy Markdown
Contributor Author

zoomxi commented Jun 2, 2025

@yariks5s Thanks for the improvement.
Now, the only failed test case is test_backup_restore_on_cluster/test_cancel_backup.py::test_short_disconnection_doesnt_stop_backup, which is flaky.
See : #80359

@zoomxi
Copy link
Copy Markdown
Contributor Author

zoomxi commented Jun 4, 2025

Can we merge this PR now? @yariks5s

@zoomxi zoomxi requested a review from yariks5s June 8, 2025 08:22
@yariks5s
Copy link
Copy Markdown
Member

Yep, now it's good to go, thank you.

@yariks5s
Copy link
Copy Markdown
Member

Looks like this PR didn't fix it: #80742 (comment)

@zoomxi
Copy link
Copy Markdown
Contributor Author

zoomxi commented Jun 12, 2025

Looks like this PR didn't fix it: #80742 (comment)

The example in #80742 has been resolved and added to the function test. There might still be other corner cases, so I'll take a closer look.

Thank you for the reminder.

@yariks5s
Copy link
Copy Markdown
Member

Revert reason: #81722 (comment)

@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-backports-created-cloud deprecated label, NOOP label Jun 13, 2025
@robot-clickhouse-ci-2 robot-clickhouse-ci-2 added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jun 13, 2025
@robot-clickhouse robot-clickhouse added the pr-must-backport-synced The `*-must-backport` labels are synced into the cloud Sync PR label Jul 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-backports-created-cloud deprecated label, NOOP pr-critical-bugfix pr-must-backport Pull request should be backported intentionally. Use this label with great care! 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logical error: 'check_in_range(result_exact_range, BoolMask::consider_only_can_be_false) == BoolMask(true, false)'.

6 participants