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.

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

expression: add implicit eval int and real for function dayname (#21806) #21850

Merged
merged 5 commits into from
Jan 27, 2021

Conversation

ti-srebot
Copy link
Contributor

cherry-pick #21806 to release-4.0


What problem does this PR solve?

Issue Number: close #19992

Problem Summary:

It's a bit long story.

MySQL has a weird behavior about function dayname() (IMO this apparently is a MySQL defect):

mysql> select dayname("1962-03-01")+1;
+-------------------------+
| dayname("1962-03-01")+1 |
+-------------------------+
|                       4 |
+-------------------------+

It evaluates as integer directly despite the function signature is actually string. Quoted of comments in MySQL source code:

TS-TODO: Item_func_dayname should be derived from Item_str_func.
In the current implementation funny things can happen:
select dayname(now())+1 -> 4

PR #9975 fixed this issue by removing the implicit cast from dayname() string to int/real, however introduced a more obvious bug:

mysql> select cast(dayname('2016-03-08') as double);                                                                                                                                                                                                                            
+---------------------------------------+                                                                                                                                                                                                                                       
| cast(dayname('2016-03-08') as double) |                           
+---------------------------------------+                                                                                               
| Tuesday                               |                           
+---------------------------------------+                                                                                                                                                                                                                                       

As shown above, unconditionally removing implicit cast gave the wrong result for non-arithmetic expressions involving function dayname().

What is changed and how it works?

What's Changed:

Don't use hasSpecialCasts anymore. Instead, mark dayname() as CanImplicitEvalInt/Real, and do "implicit" evalInt/Real in the actual cast functions.

Besides, since filter operator has its own path to evaluate expressions into boolean, modify it too to do "implicit" evalInt/Real.

How it Works:
Adding implicit evalInt/Real path makes cast functions directly evaluate dayname() as int/real, which further involve into whatever evaluation in the upper level of the expression tree.

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn: none
  • Need to cherry-pick to the release branch: release-4.0

Check List

Tests

  • Unit test

Side effects

  • Performance regression
    • Consumes more CPU

Release note

  • Fix compatibility issue with MySQL for function dayname

Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot
Copy link
Contributor Author

/run-all-tests

@ti-srebot
Copy link
Contributor Author

@zanmato1984 you're already a collaborator in bot's repo.

@jebter jebter modified the milestones: 4.0.0, v4.0.10 Jan 7, 2021
@jebter jebter modified the milestones: v4.0.10, v4.0.11 Jan 18, 2021
@@ -331,20 +331,36 @@ func VecEvalBool(ctx sessionctx.Context, exprList CNFExprs, input *chunk.Chunk,
isZero := allocZeroSlice(n)
defer deallocateZeroSlice(isZero)
for _, expr := range exprList {
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

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

Please resolve conflicts @zanmato1984

@qw4990
Copy link
Contributor

qw4990 commented Jan 26, 2021

Could you please help to resolve conflicts? @ichn-hu @lzmhhh123

Copy link
Contributor

@qw4990 qw4990 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 added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 27, 2021
@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-all-tests

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-unit-test

2 similar comments
@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-unit-test

@qw4990
Copy link
Contributor

qw4990 commented Jan 27, 2021

/run-unit-test

@ti-srebot ti-srebot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 27, 2021
@qw4990 qw4990 merged commit 456acfa into pingcap:release-4.0 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug. type/4.0-cherry-pick
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants