expression: Improved precision of FROM_UNIXTIME(char) #35184 #37788

keeplearning20221 · 2022-09-13T13:59:02Z

What problem does this PR solve?

Issue Number: ref #35184

Problem Summary:
fix FROM_UNIXTIME(char/double) is not compatible with mysql

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

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

None

…gcap#35184

ti-chi-bot · 2022-09-13T13:59:03Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

qw4990
windtalker

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

modified: types/field_type.go

… into fixbug35184

modified: parser/mysql/type.go modified: types/datum.go

TonsnakeLin · 2022-09-16T09:11:21Z

types/datum.go

@@ -1468,6 +1469,14 @@ func (d *Datum) convertToMysqlDecimal(sc *stmtctx.StatementContext, target *Fiel
 // ProduceDecWithSpecifiedTp produces a new decimal according to `flen` and `decimal`.
 func ProduceDecWithSpecifiedTp(dec *MyDecimal, tp *FieldType, sc *stmtctx.StatementContext) (_ *MyDecimal, err error) {
 	flen, decimal := tp.GetFlen(), tp.GetDecimal()
+
+	//If string to decimal, determine the precision for each string


Is there any possible that (tp.GetFlag() & mysql.StringToDecimalFlag) is true and flen != UnspecifiedLength && decimal != UnspecifiedLength is false?

NO, None of these types （mysql.TypeString, mysql.TypeVarchar, mysql.TypeTinyBlob, mysql.TypeMediumBlob, mysql.TypeLongBlob, mysql.TypeBlob） GetFlen（） == -1

you'd better to decrease cyclomatic complexity. I mean that you added if {} block before origin if flen != UnspecifiedLength && decimal != UnspecifiedLength {}, the code looks a little hard to read and maintain.

TonsnakeLin · 2022-09-16T14:42:57Z

expression/integration_test.go

+	result.Check(testkit.Rows(unixTime1, unixTime2))
+
+	tk.MustExec("drop table if exists ft;")
+	tk.MustExec("create table ft (tint int, tdou double, tdec decimal(22,9),tchar longblob);")


Could you remove the irrelevant columns and tail semicolon

TonsnakeLin · 2022-09-16T14:45:17Z

No obvious problem was found, but please ask the developer of the relevant code to review it again.

modified: expression/integration_test.go modified: planner/core/plan_to_pb.go modified: types/datum.go

modified: expression/builtin_time.go modified: expression/builtin_time_vec.go modified: expression/expr_to_pb.go modified: expression/expression.go modified: parser/mysql/type.go

windtalker · 2022-09-21T07:27:54Z

expression/builtin_time.go

@@ -1661,6 +1661,10 @@ func (c *fromUnixTimeFunctionClass) getFunction(ctx sessionctx.Context, args []E
 		return nil, err
 	}

+	if fieldString(arg0Tp.GetType()) {


why not use isArg0Str?

TypeUnspecified is not the expected type，which satisfies types.IsString.
and TypeVarString is an input parameter, no need to adjust the precision .

windtalker · 2022-09-21T07:36:05Z

expression/builtin_time.go

+	if ok {
+		// FixUnixtimePrecision is used to adjust FromUnixTime precision #Fixbug35184
+		if x.FuncName.L == ast.Cast {
+			if x.RetType.GetDecimal() == 0 {


why need to check x.RetType.GetDecimal(), sometime the decimal can be -1, and it also need to be updated.

why need check if it is cast function? I think just check if the return type if decimal is ok

1）string type must be equal to 0
2）The current application scenario is cast

windtalker · 2022-09-21T07:36:47Z

expression/integration_test.go

+	result.Check(testkit.Rows("1973-11-30 08:38:10.1"))
+
+	result = tk.MustQuery("SELECT FROM_UNIXTIME(tchar,'%Y%m%d') from ft where FROM_UNIXTIME(tchar)= '1973-11-30 08:38:10.123400' ")
+	result.Check(testkit.Rows("19731130"))


also add a test that the input is string literal?

keeplearning20221 · 2022-09-21T10:42:49Z

/run-unit-test

keeplearning20221 · 2022-09-21T11:47:31Z

/run-unit-test

keeplearning20221 · 2022-09-21T14:01:32Z

/run-unit-test

keeplearning20221 · 2022-09-21T14:54:31Z

/run-build

keeplearning20221 · 2022-09-21T15:22:11Z

/run-unit-test

windtalker · 2022-09-22T00:55:39Z

expression/builtin_time.go

+		if ok {
+			//used to adjust FromUnixTime precision #Fixbug35184
+			if x.FuncName.L == ast.Cast {
+				if x.RetType.GetDecimal() == 0 {


Add extra check to make sure the x.RetType is decimal ?

windtalker

LGTM

windtalker · 2022-09-22T03:44:56Z

/merge

ti-chi-bot · 2022-09-22T03:44:59Z

This pull request has been accepted and is ready to merge.

Commit hash: 7dbfbdf

ti-chi-bot · 2022-09-22T04:05:15Z

@keeplearning20221: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

expression,parser,types:Improved precision of FROM_UNIXTIME(char) pin…

d529d56

…gcap#35184

ti-chi-bot added do-not-merge/invalid-title release-note-none size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2022

keeplearning20221 changed the title ~~expression,parser,types:Improved precision of FROM_UNIXTIME(char) #35184~~ expression[,parser,types]:Improved precision of FROM_UNIXTIME(char) #35184 Sep 13, 2022

keeplearning20221 changed the title ~~expression[,parser,types]:Improved precision of FROM_UNIXTIME(char) #35184~~ expression[,parser,mysql,types]:Improved precision of FROM_UNIXTIME(char) #35184 Sep 13, 2022

modified: types/datum.go

79fe416

modified: types/field_type.go

keeplearning20221 changed the title ~~expression[,parser,mysql,types]:Improved precision of FROM_UNIXTIME(char) #35184~~ expression [, parser, mysql, types]: Improved precision of FROM_UNIXTIME(char) #35184 Sep 13, 2022

ti-chi-bot removed the do-not-merge/invalid-title label Sep 13, 2022

keeplearning20221 and others added 7 commits September 13, 2022 23:01

modified: ../types/datum.go

9ce1ce7

Merge branch 'master' into fixbug35184

d32dcc3

modified: expression/builtin_cast.go

6577ffb

Merge branch 'fixbug35184' of https://github.com/keeplearning20221/tidb…

2fe0964

… into fixbug35184

modified: expression/builtin_cast.go

ff32c9e

modified: expression/integration_test.go

addbbfe

modified: expression/builtin_cast.go

a9b5a64

modified: parser/mysql/type.go modified: types/datum.go

ti-chi-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 14, 2022

modified: expression/builtin_cast.go

bf47465

cfzjywxk requested review from qw4990, windtalker and lance6716 September 16, 2022 07:05

TonsnakeLin reviewed Sep 16, 2022

View reviewed changes

modified: expression/expression.go

b93afaf

modified: expression/integration_test.go modified: planner/core/plan_to_pb.go modified: types/datum.go

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 17, 2022

AilinKid mentioned this pull request Sep 17, 2022

planner, executor: implement the null-aware antiSemiJoin and null-aware antiLeftOuterSemiJoin (hash join with inner build) #37512

Merged

12 tasks

keeplearning20221 added 4 commits September 21, 2022 10:22

modified: expression/builtin_cast.go

2bb9eb5

modified: expression/builtin_time.go modified: expression/builtin_time_vec.go modified: expression/expr_to_pb.go modified: expression/expression.go modified: parser/mysql/type.go

modified: expression/builtin_time.go

d214bc5

modified: planner/core/testdata/integration_suite_out.json

1d4ef17

modified: expression/builtin_time_vec.go

a2ace25

windtalker reviewed Sep 21, 2022

View reviewed changes

keeplearning20221 and others added 2 commits September 21, 2022 16:24

modified: expression/builtin_time.go

c8a9e41

Merge branch 'master' into fixbug35184

666edb2

keeplearning20221 changed the title ~~expression, mysql: Improved precision of FROM_UNIXTIME(char) #35184~~ expression: Improved precision of FROM_UNIXTIME(char) #35184 Sep 21, 2022

Merge branch 'master' into fixbug35184

e13f8df

Merge branch 'master' into fixbug35184

9a12b7c

Merge branch 'master' into fixbug35184

ff922c9

windtalker reviewed Sep 22, 2022

View reviewed changes

modified: expression/builtin_time.go

7dbfbdf

windtalker approved these changes Sep 22, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Sep 22, 2022

qw4990 approved these changes Sep 22, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Sep 22, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Sep 22, 2022

ti-chi-bot added 2 commits September 22, 2022 11:45

Merge branch 'master' into fixbug35184

44113c9

Merge branch 'master' into fixbug35184

ba5581f

ti-chi-bot merged commit b2b8c56 into pingcap:master Sep 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression: Improved precision of FROM_UNIXTIME(char) #35184 #37788

expression: Improved precision of FROM_UNIXTIME(char) #35184 #37788

keeplearning20221 commented Sep 13, 2022 •

edited

ti-chi-bot commented Sep 13, 2022 •

edited

TonsnakeLin Sep 16, 2022

keeplearning20221 Sep 16, 2022

TonsnakeLin Sep 16, 2022

keeplearning20221 Sep 17, 2022

TonsnakeLin Sep 16, 2022

keeplearning20221 Sep 17, 2022

TonsnakeLin commented Sep 16, 2022

windtalker Sep 21, 2022

keeplearning20221 Sep 21, 2022

windtalker Sep 21, 2022

keeplearning20221 Sep 21, 2022

windtalker Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

windtalker Sep 22, 2022

windtalker left a comment

windtalker commented Sep 22, 2022

ti-chi-bot commented Sep 22, 2022

ti-chi-bot commented Sep 22, 2022

expression: Improved precision of FROM_UNIXTIME(char) #35184 #37788

expression: Improved precision of FROM_UNIXTIME(char) #35184 #37788

Conversation

keeplearning20221 commented Sep 13, 2022 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Sep 13, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TonsnakeLin commented Sep 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

keeplearning20221 commented Sep 21, 2022

Choose a reason for hiding this comment

windtalker left a comment

Choose a reason for hiding this comment

windtalker commented Sep 22, 2022

ti-chi-bot commented Sep 22, 2022

ti-chi-bot commented Sep 22, 2022

keeplearning20221 commented Sep 13, 2022 •

edited

ti-chi-bot commented Sep 13, 2022 •

edited