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

planner/core: add DefaultExpr support for expressionRewriter #8540

Merged
merged 6 commits into from Jan 2, 2019

Conversation

Projects
None yet
5 participants
@bb7133
Copy link
Contributor

bb7133 commented Dec 1, 2018

What problem does this PR solve?

This PR simply fix a compatibility issue about default() function in expressionRewriter:

mysql> create table t(a int default 5);
Query OK, 0 rows affected (0.00 sec)

mysql> insert into t values (1);
Query OK, 1 row affected (0.00 sec)

mysql> select default(a) from t;
ERROR 1105 (HY000): UnknownType: *ast.DefaultExpr
mysql> update t set a=default(a);
ERROR 1105 (HY000): UnknownType: *ast.DefaultExpr

default() function should return the default value of a given column:
https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_default

What is changed and how it works?

In expressionRewriter, this PR adds a branch to handle ast.DefaultExpr to avoid the UnknownType error and fix this issue.

Check List

Tests

  • Integration test

Related changes

  • Nothing else

This change is Reviewable

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from 4388c08 to 7f89ad9 Dec 2, 2018

@bb7133 bb7133 referenced this pull request Dec 2, 2018

Open

Add remaining miscellaneous functions #8439

14 of 17 tasks complete

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from 7f89ad9 to 2cdd49f Dec 2, 2018

Show resolved Hide resolved planner/core/expression_rewriter_test.go
Show resolved Hide resolved planner/core/expression_rewriter.go Outdated
}

func hasCurrentTimestampDefault(col *table.Column) bool {
if col.Tp != mysql.TypeTimestamp && col.Tp != mysql.TypeDatetime {

This comment has been minimized.

@XuHuaiyu

XuHuaiyu Dec 3, 2018

Contributor

We do not really need this, check the name is enough and clearer.

This comment has been minimized.

@bb7133

bb7133 Dec 3, 2018

Contributor

We need to check the types(for example, to avoid varchar column with 'current_timestamp' default string).
What's more, I just found timestamp and datetime behaves differently here, so a code update is needed, thanks anyway!

Show resolved Hide resolved planner/core/expression_rewriter.go Outdated

@zz-jason zz-jason requested review from zz-jason and winoros Dec 5, 2018

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from 2cdd49f to 0e19852 Dec 5, 2018

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from 1c1fe38 to dbee759 Dec 12, 2018

@bb7133

This comment has been minimized.

Copy link
Contributor

bb7133 commented Dec 13, 2018

hi @zz-jason , I've updated code, it looks much better now! Thank you very much

@winoros
Copy link
Member

winoros left a comment

rest lgtm

Show resolved Hide resolved planner/core/expression_rewriter.go Outdated

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from 0e68a66 to d1b4406 Dec 27, 2018

@bb7133

This comment has been minimized.

Copy link
Contributor

bb7133 commented Dec 28, 2018

hi @winoros @eurekaka , I've updated the code, PTAL, thanks.

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from d1b4406 to bbd61b7 Dec 29, 2018

@eurekaka
Copy link
Contributor

eurekaka left a comment

LGTM

@eurekaka

This comment has been minimized.

Copy link
Contributor

eurekaka commented Dec 29, 2018

bb7133 added some commits Dec 1, 2018

@bb7133 bb7133 force-pushed the bb7133:bb7133/fix_default_expr branch from bbd61b7 to 2d0c469 Jan 2, 2019

@zz-jason
Copy link
Member

zz-jason left a comment

LGTM

@zz-jason

This comment has been minimized.

Copy link
Member

zz-jason commented Jan 2, 2019

/run-all-tests

@zz-jason zz-jason added status/LGT2 and removed status/LGT1 labels Jan 2, 2019

@zz-jason zz-jason merged commit 1232db5 into pingcap:master Jan 2, 2019

12 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment