Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expression: modify the mean result of time type #21514

Merged
merged 15 commits into from
Dec 21, 2020
Merged

expression: modify the mean result of time type #21514

merged 15 commits into from
Dec 21, 2020

Conversation

ou-bing
Copy link
Contributor

@ou-bing ou-bing commented Dec 6, 2020 •
edited
Loading

What problem does this PR solve?

Issue Number: close #17726

What is changed and how it works?

What's Changed:

modify the mean result of time type.

How it Works:

Force 4 decimal places.

Release note

  • modify the mean result of time type

@ou-bing ou-bing requested a review from a team as a code owner December 6, 2020 14:24
@ou-bing ou-bing requested review from lzmhhh123 and removed request for a team December 6, 2020 14:24
@ichn-hu ichn-hu mentioned this pull request Dec 6, 2020
Closed
@ou-bing
Copy link
Contributor Author

ou-bing commented Dec 7, 2020 •
edited
Loading

My test case failed, because, in the test environment, the process of reading query results is inconsistent with that in the production environment。This mainly refers to the difference between functions ResultSetToStringSlice and dumpTextRow.

There is no problem with the following test cases executed on the command line.

Test case:

	tk := testkit.NewTestKit(c, s.store)
	tk.MustExec("use test")
	tk.MustExec("drop table if exists t0")
	tk.MustExec("create table t0 (c1 DATE, c2 TIME, c3 DATETIME, c4 TIMESTAMP)")
	tk.MustExec("insert into t0 values ('1000-01-01', '-838:59:59', '1000-01-01 00:00:00', '1970-01-01 08:00:01')")
	tk.MustExec("insert into t0 values ('9999-12-31', '838:59:59', '9999-12-31 23:59:59', '2038-01-19 11:14:07')")
	result := tk.MustQuery("select avg(c1), avg(c2), avg(c3), avg(c4) from t0")
	result.Check(testkit.Rows("54995666.0000 0.0000 54995666117979.5000 20040110095704.0000"))

Test result：

----------------------------------------------------------------------
FAIL: integration_test.go:8061: testIntegrationSuite.TestIssue17726

integration_test.go:8069:
    result.Check(testkit.Rows("54995666.0000 0.0000 54995666117979.5000 20040110095704.0000"))
/Users/oubing/Project/Go/tidb/util/testkit/testkit.go:63:
    res.c.Assert(resBuff.String(), check.Equals, needBuff.String(), res.comment)
... obtained string = "[54995666 0 54995666117979.5 20040110095704]\n"
... expected string = "[54995666.0000 0.0000 54995666117979.5000 20040110095704.0000]\n"
... sql:select avg(c1), avg(c2), avg(c3), avg(c4) from t0, args:[]

lzmhhh123
lzmhhh123 reviewed Dec 7, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

What about not directly compare the string result. Parse the float to compare.

@lzmhhh123
Copy link
Contributor

/reward 300

@ti-challenge-bot
Copy link

Reward success.

@ou-bing
Copy link
Contributor Author

ou-bing commented Dec 8, 2020

What about not directly compare the string result. Parse the float to compare.

Maybe it doesn't make any sense? Because it can pass the test without any modification.

@ou-bing
Copy link
Contributor Author

ou-bing commented Dec 10, 2020

The test case TestAsyncCommitWithSchemaChange is passed in my local environment😂

lzmhhh123
lzmhhh123 reviewed Dec 10, 2020
View reviewed changes
expression/integration_test.go
tk.MustExec("insert into t0 values ('1000-01-01', '-838:59:59', '1000-01-01 00:00:00', '1970-01-01 08:00:01')")
tk.MustExec("insert into t0 values ('9999-12-31', '838:59:59', '9999-12-31 23:59:59', '2038-01-19 11:14:07')")
result := tk.MustQuery("select avg(c1), avg(c2), avg(c3), avg(c4) from t0")
result.Check(testkit.Rows("54995666 0 54995666117979.5 20040110095704"))
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems not as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you want me to compare float values?
Are there any related tools in the test suite...

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, the unit test doesn't go through the client connection. The manual test with the MySQL client is right.

@XuHuaiyu XuHuaiyu added the type/bugfix This PR fixes a bug. label Dec 11, 2020
@XuHuaiyu XuHuaiyu requested a review from lzmhhh123 December 15, 2020 06:47
lzmhhh123
lzmhhh123 reviewed Dec 15, 2020
View reviewed changes
Copy link
Contributor

@lzmhhh123 lzmhhh123 left a comment

Choose a reason for hiding this comment

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

LGTM

expression/integration_test.go
tk.MustExec("insert into t0 values ('1000-01-01', '-838:59:59', '1000-01-01 00:00:00', '1970-01-01 08:00:01')")
tk.MustExec("insert into t0 values ('9999-12-31', '838:59:59', '9999-12-31 23:59:59', '2038-01-19 11:14:07')")
result := tk.MustQuery("select avg(c1), avg(c2), avg(c3), avg(c4) from t0")
result.Check(testkit.Rows("54995666 0 54995666117979.5 20040110095704"))
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, the unit test doesn't go through the client connection. The manual test with the MySQL client is right.

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Dec 15, 2020
@XuHuaiyu XuHuaiyu requested a review from wshwsh12 December 21, 2020 08:05
wshwsh12
wshwsh12 reviewed Dec 21, 2020
View reviewed changes
Copy link
Contributor

@wshwsh12 wshwsh12 left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Dec 21, 2020
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Dec 21, 2020
@wshwsh12
Copy link
Contributor

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Dec 21, 2020
@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 21788

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit e8fb3ea into pingcap:master Dec 21, 2020
@ti-challenge-bot
Copy link

@ou-bing, Congratulations, you get 300 in this PR, and your total score is 600 in challenge program.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wshwsh12 wshwsh12 wshwsh12 left review comments

@lzmhhh123 lzmhhh123 lzmhhh123 left review comments

@ti-srebot ti-srebot ti-srebot approved these changes

Assignees
No one assigned
Labels
component/expression status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

P3-[4.0 Bug Hunting]-[Aggregate Function]-no warning and wrong result on date column
5 participants
@ou-bing @lzmhhh123 @wshwsh12 @ti-srebot @XuHuaiyu