Fix decimal floor/ceil (#10365) by ChangRui-Ryan · Pull Request #10364 · pingcap/tiflash

ChangRui-Ryan · 2025-08-25T10:00:05Z

What problem does this PR solve?

Issue Number: close #10365, close #3496, ref pingcap/tidb#63086

Problem Summary:

What is changed and how it works?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

None

pingcap-cla-assistant · 2025-08-25T10:00:18Z

All committers have signed the CLA.

JaySon-Huang · 2025-08-27T05:59:08Z

https://do.pingcap.net/jenkins/blue/organizations/jenkins/pingcap%2Ftiflash%2Fpull_integration_test/detail/pull_integration_test/4670/pipeline
@ChangRui-Ryan Format the files using the script here pls
https://github.com/pingcap/tiflash?tab=readme-ov-file#contributing

windtalker · 2025-08-27T09:49:37Z

dbms/src/Functions/FunctionsRound.h

+            using Op = IntegerRoundingComputation<NativeType, rounding_mode, ScaleMode::Negative>;
+            auto scale_factor = intExp10OfSize<NativeType>(decimal_scale);
+
+            if constexpr (std::is_same_v<T, OutputType>)


In which case will the output be Int64, and in which case will the output type the same as T?

For decimal(M, N), if M-N > 18, use T; otherwise use Int64. Because Int64 is sufficient to represent 18-digit integers. TiDB will make decision.

windtalker · 2025-08-27T09:51:14Z

dbms/src/Functions/FunctionsRound.h

+
+            if constexpr (std::is_same_v<T, OutputType>)
+            {
+                Op::compute(&in->value, scale_factor, &out->value);


If the output type is decimal, will the return type of ceil/floor keep its input type's scale, or the result type's scale is always 0?

will keep its input type's scale.

Are you sure of this? Because from TiDB's code, it seems if the return type is decimal, the scale is always 0:
https://github.com/pingcap/tidb/blob/ad52c8e05b49602888f08db4a81cfcffbb21e555/pkg/expression/builtin_math.go#L732-L735

From getReturnTypeImpl of FunctionRounding class, we can see that return type is just arguments[0], and I didn't change that in this pr. Maybe some other places change the scale to 0 after. Let me try to find out.

that is because after TiFlash's function, we will add extra cast to keep the return type the same as TiDB. But for this particular function, I think we can adopt TiDB's return type infer logic in TiFlash so we don't need to add extra cast after the function.

OK, I will optimize that.

windtalker · 2025-09-02T07:13:54Z

dbms/src/Functions/FunctionsRound.h

+        // So, we only handle ScaleMode::Zero here.
+        if constexpr (scale_mode == ScaleMode::Zero)
+        {
+            auto scale_factor = intExp10OfSize<NativeType>(decimal_scale);


why not use scale_factor as the last parameter of compute, so we don't need calcuate scale_factor for every input row

Good suggestion, I will refactor this part.
Originally, I didn’t want to make major change, like modifying function interface.

ChangRui-Ryan · 2025-09-03T03:33:54Z

/retest

windtalker

lgtm

ti-chi-bot · 2025-09-05T03:15:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: windtalker, wshwsh12

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [windtalker,wshwsh12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ti-chi-bot · 2025-09-05T03:15:19Z

[LGTM Timeline notifier]

Timeline:

2025-09-03 07:45:54.673315145 +0000 UTC m=+944646.551146098: ☑️ agreed by windtalker.
2025-09-05 03:15:19.023955617 +0000 UTC m=+149663.007053065: ☑️ agreed by wshwsh12.

ti-chi-bot · 2025-09-08T01:20:29Z

In response to a cherrypick label: new pull request created to branch release-8.5: #10412.

ti-chi-bot · 2025-09-08T01:20:33Z

In response to a cherrypick label: new pull request created to branch release-8.1: #10413.

ti-chi-bot · 2025-09-08T01:20:38Z

In response to a cherrypick label: new pull request created to branch release-7.5: #10414.

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-09-08T01:20:42Z

In response to a cherrypick label: new pull request created to branch release-7.1: #10415.
But this PR has conflicts, please resolve them!

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot · 2025-09-08T01:20:46Z

In response to a cherrypick label: new pull request created to branch release-6.5: #10416.
But this PR has conflicts, please resolve them!

close #3496, close #10365, ref pingcap/tidb#63086 Co-authored-by: ChangRui-Ryan <changrui82@gmail.com>

ti-chi-bot bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/needs-linked-issue size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 25, 2025

ti-chi-bot bot removed the do-not-merge/needs-linked-issue label Aug 26, 2025

ChangRui-Ryan force-pushed the decimal_floor branch from 36aecb4 to 75198fc Compare August 26, 2025 09:23

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/needs-triage-completed and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 26, 2025

ChangRui-Ryan force-pushed the decimal_floor branch from 75198fc to 68372fc Compare August 26, 2025 10:47

ti-chi-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 26, 2025

ChangRui-Ryan force-pushed the decimal_floor branch 2 times, most recently from 799bf98 to 26fbbb9 Compare August 27, 2025 03:43

ti-chi-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/needs-triage-completed labels Aug 27, 2025

ChangRui-Ryan changed the title ~~Fix decimal floor/ceil (#63086)~~ Fix decimal floor/ceil (#10365) Aug 27, 2025

ChangRui-Ryan force-pushed the decimal_floor branch from 26fbbb9 to d7aaefc Compare August 27, 2025 06:11

windtalker reviewed Aug 27, 2025

View reviewed changes

ChangRui-Ryan force-pushed the decimal_floor branch 2 times, most recently from bf4b947 to fb9278c Compare August 29, 2025 07:28

windtalker reviewed Sep 2, 2025

View reviewed changes

Fix decimal floor/ceil (pingcap#10365)

ef767b0

ChangRui-Ryan force-pushed the decimal_floor branch from fb9278c to ef767b0 Compare September 3, 2025 02:30

windtalker approved these changes Sep 3, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels Sep 3, 2025

wshwsh12 approved these changes Sep 5, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Sep 5, 2025

ti-chi-bot bot merged commit f5f30b8 into pingcap:master Sep 5, 2025
7 checks passed

ti-chi-bot mentioned this pull request Sep 8, 2025

Fix decimal floor/ceil (#10365) (#10364) #10412

Merged

12 tasks

ti-chi-bot mentioned this pull request Sep 8, 2025

Fix decimal floor/ceil (#10365) (#10364) #10413

Open

12 tasks

ti-chi-bot mentioned this pull request Sep 8, 2025

Fix decimal floor/ceil (#10365) (#10364) #10414

Open

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 8, 2025

This is an automated cherry-pick of pingcap#10364

c4ccf0f

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Sep 8, 2025

Fix decimal floor/ceil (#10365) (#10364) #10415

Open

12 tasks

ti-chi-bot pushed a commit to ti-chi-bot/tiflash that referenced this pull request Sep 8, 2025

This is an automated cherry-pick of pingcap#10364

b7ff7c2

Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>

ti-chi-bot mentioned this pull request Sep 8, 2025

Fix decimal floor/ceil (#10365) (#10364) #10416

Closed

12 tasks

ti-chi-bot bot pushed a commit that referenced this pull request Sep 10, 2025

Fix decimal floor/ceil (#10365) (#10364) (#10412)

59bb758

close #3496, close #10365, ref pingcap/tidb#63086 Co-authored-by: ChangRui-Ryan <changrui82@gmail.com>

JaySon-Huang mentioned this pull request Sep 10, 2025

mac may build failed #10422

Closed

Conversation

ChangRui-Ryan commented Aug 25, 2025 • edited by JaySon-Huang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

pingcap-cla-assistant bot commented Aug 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaySon-Huang commented Aug 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChangRui-Ryan commented Sep 3, 2025

Uh oh!

windtalker left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented Sep 5, 2025

Uh oh!

ti-chi-bot bot commented Sep 5, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

ti-chi-bot commented Sep 8, 2025

Uh oh!

ti-chi-bot commented Sep 8, 2025

Uh oh!

ti-chi-bot commented Sep 8, 2025

Uh oh!

ti-chi-bot commented Sep 8, 2025

Uh oh!

ti-chi-bot commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ChangRui-Ryan commented Aug 25, 2025 •

edited by JaySon-Huang

Loading

pingcap-cla-assistant bot commented Aug 25, 2025 •

edited

Loading