expression: round the frac part for `ParseTimeFromFloat64` #56340

gengliqi · 2024-09-26T09:22:17Z

What problem does this PR solve?

Issue Number: close #56339

Problem Summary:
See #56339

What changed and how does it work?

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No need to test
- I checked and no code files have been changed.

Side effects

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

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

None

tiprow · 2024-09-26T09:22:35Z

Hi @gengliqi. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

gengliqi · 2024-09-26T09:34:01Z

/check-issue-triage-complete

codecov · 2024-09-26T09:38:47Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.4178%. Comparing base (a22b842) to head (307e813).
Report is 5 commits behind head on master.

Additional details and impacted files

@@               Coverage Diff                @@
##             master     #56340        +/-   ##
================================================
+ Coverage   72.8488%   73.4178%   +0.5689%     
================================================
  Files          1672       1672                
  Lines        462666     463709      +1043     
================================================
+ Hits         337047     340445      +3398     
+ Misses       104821     102480      -2341     
+ Partials      20798      20784        -14

Flag	Coverage Δ
integration	`43.3662% <100.0000%> (?)`
unit	`72.2300% <100.0000%> (+0.0083%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
dumpling	`52.7673% <ø> (ø)`
parser	`∅ <ø> (∅)`
br	`45.1379% <ø> (+0.0153%)`	⬆️

dveeden · 2024-10-04T15:47:15Z

@mjonss this might be related to (or conflict with) what you are working on.

mjonss · 2024-10-04T16:40:49Z

@mjonss this might be related to (or conflict with) what you are working on.

Yes, if this is merged, then I need to update #56442 as well.

mjonss · 2024-10-04T16:52:19Z

pkg/types/time_test.go

@@ -1814,7 +1814,8 @@ func TestParseTimeFromFloat64(t *testing.T) {
 		{0.0, mysql.TypeDate, 0, 0, 0, 0, 0, 0, 0, nil},
 		{20000102030405, mysql.TypeDatetime, 2000, 1, 2, 3, 4, 5, 0, nil},
 		{20000102030405.015625, mysql.TypeDatetime, 2000, 1, 2, 3, 4, 5, 15625, nil},
-		{20000102030405.0078125, mysql.TypeDatetime, 2000, 1, 2, 3, 4, 5, 7812, nil},
+		{20000102030405.0078125, mysql.TypeDatetime, 2000, 1, 2, 3, 4, 5, 7813, nil},


The significant digits of the float fractional part is only '.008' I guess, so does it really matter if it is '.007812' or '.007813' ?
Just try this with your PR:

{20000102030405.0078139, mysql.TypeDatetime, 2000, 1, 2, 3, 4, 5, 7814, nil},

Yes. The fractional part is only .008 so the fractional part of your case is still 7813.
As #56339 (comment) said, I just want to make this behavior consistent with TiKV and MySQL.

OK, then I see.
Can you please add tests that will test TiKV/TiFlash/UniStore handling of co-processor calls for this (if exists), so we can show that all four implementations are the same?

I added an integration test to test TiDB(Unistore)/TiKV. TiFlash does not support date_add/date_sub functions with float as the first parameter for now.

@mjonss is this now resolved?

@gengliqi I only see a single integration test. Are you sure it is testing with both TiKV and Unistore?

Yes, the check_dev test runs these integration tests with Unistore while the check_dev_2 runs with a real TiKV.

Signed-off-by: gengliqi <gengliqiii@gmail.com>

ti-chi-bot · 2024-10-18T10:08:06Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dveeden · 2024-11-18T06:16:01Z

@gengliqi could you fix the conflict?

Signed-off-by: gengliqi <gengliqiii@gmail.com>

gengliqi · 2024-11-18T06:43:00Z

@gengliqi could you fix the conflict?

Fixed. Thanks for reminding me.

dveeden · 2024-11-18T07:06:01Z

/ok-to-test

dveeden · 2024-11-18T08:27:49Z

@gengliqi any idea about why the checks are failing?

gengliqi · 2024-11-18T08:38:05Z

/retest

gengliqi · 2024-11-18T09:39:43Z

/retest

gengliqi · 2024-11-18T09:40:14Z

@gengliqi any idea about why the checks are failing?

It seems it does not relate to this PR.

ti-chi-bot · 2024-11-18T11:07:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dveeden, mjonss

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dveeden,mjonss]

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 · 2024-11-18T11:07:43Z

[LGTM Timeline notifier]

Timeline:

2024-11-18 07:04:23.829353679 +0000 UTC m=+858226.020222676: ☑️ agreed by dveeden.
2024-11-18 11:07:42.446226183 +0000 UTC m=+872824.637095180: ☑️ agreed by mjonss.

ti-chi-bot bot added do-not-merge/needs-triage-completed release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 26, 2024

ti-chi-bot bot removed the do-not-merge/needs-triage-completed label Sep 26, 2024

mjonss reviewed Oct 4, 2024

View reviewed changes

u

5d6be93

Signed-off-by: gengliqi <gengliqiii@gmail.com>

gengliqi force-pushed the fix-date-add-sub-float branch from a3f949b to 5d6be93 Compare October 9, 2024 07:41

add one integration test

142eba3

Signed-off-by: gengliqi <gengliqiii@gmail.com>

ti-chi-bot bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 11, 2024

ti-chi-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2024

fix conflict

307e813

Signed-off-by: gengliqi <gengliqiii@gmail.com>

dveeden approved these changes Nov 18, 2024

View reviewed changes

ti-chi-bot bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 18, 2024

ti-chi-bot bot added the ok-to-test Indicates a PR is ready to be tested. label Nov 18, 2024

dveeden removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2024

mjonss approved these changes Nov 18, 2024

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Nov 18, 2024

ti-chi-bot bot merged commit 9512c96 into pingcap:master Nov 18, 2024
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: round the frac part for `ParseTimeFromFloat64` #56340

expression: round the frac part for `ParseTimeFromFloat64` #56340

gengliqi commented Sep 26, 2024 •

edited

Loading

tiprow bot commented Sep 26, 2024

gengliqi commented Sep 26, 2024

codecov bot commented Sep 26, 2024 •

edited

Loading

dveeden commented Oct 4, 2024

mjonss commented Oct 4, 2024

mjonss Oct 4, 2024

gengliqi Oct 9, 2024

mjonss Oct 9, 2024

gengliqi Oct 11, 2024 •

edited

Loading

dveeden Nov 18, 2024

dveeden Nov 18, 2024

gengliqi Nov 18, 2024

ti-chi-bot bot commented Oct 18, 2024

dveeden commented Nov 18, 2024

gengliqi commented Nov 18, 2024

dveeden commented Nov 18, 2024

dveeden commented Nov 18, 2024

gengliqi commented Nov 18, 2024

gengliqi commented Nov 18, 2024

gengliqi commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

expression: round the frac part for ParseTimeFromFloat64 #56340

expression: round the frac part for ParseTimeFromFloat64 #56340

Conversation

gengliqi commented Sep 26, 2024 • edited Loading

What problem does this PR solve?

What changed and how does it work?

Check List

Release note

tiprow bot commented Sep 26, 2024

gengliqi commented Sep 26, 2024

codecov bot commented Sep 26, 2024 • edited Loading

Codecov Report

dveeden commented Oct 4, 2024

mjonss commented Oct 4, 2024

mjonss Oct 4, 2024

Choose a reason for hiding this comment

gengliqi Oct 9, 2024

Choose a reason for hiding this comment

mjonss Oct 9, 2024

Choose a reason for hiding this comment

gengliqi Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

dveeden Nov 18, 2024

Choose a reason for hiding this comment

dveeden Nov 18, 2024

Choose a reason for hiding this comment

gengliqi Nov 18, 2024

Choose a reason for hiding this comment

ti-chi-bot bot commented Oct 18, 2024

dveeden commented Nov 18, 2024

gengliqi commented Nov 18, 2024

dveeden commented Nov 18, 2024

dveeden commented Nov 18, 2024

gengliqi commented Nov 18, 2024

gengliqi commented Nov 18, 2024

gengliqi commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

ti-chi-bot bot commented Nov 18, 2024

[LGTM Timeline notifier]

expression: round the frac part for `ParseTimeFromFloat64` #56340

expression: round the frac part for `ParseTimeFromFloat64` #56340

gengliqi commented Sep 26, 2024 •

edited

Loading

codecov bot commented Sep 26, 2024 •

edited

Loading

gengliqi Oct 11, 2024 •

edited

Loading