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: rewrite builtin function: INTDIV #4213

Merged
merged 32 commits into from Sep 5, 2017

Conversation

Projects
None yet
5 participants
@lkk2003rty
Contributor

lkk2003rty commented Aug 16, 2017

to #3851

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 18, 2017

Member

hi @lkk2003rty, any update ?

Member

zz-jason commented Aug 18, 2017

hi @lkk2003rty, any update ?

@lkk2003rty

@zz-jason
just update as comments said

  • remove case opcode.Mul
  • refactor as ComputeIntDiv
  • add more tests
@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 23, 2017

Member

@lkk2003rty please resolve conflicts

Member

zz-jason commented Aug 23, 2017

@lkk2003rty please resolve conflicts

@lkk2003rty

This comment has been minimized.

Show comment
Hide comment
@lkk2003rty

lkk2003rty Aug 23, 2017

Contributor

resolve it. @zz-jason

Contributor

lkk2003rty commented Aug 23, 2017

resolve it. @zz-jason

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Aug 28, 2017

Contributor

PTAL @zz-jason

Contributor

XuHuaiyu commented Aug 28, 2017

PTAL @zz-jason

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 29, 2017

Member

@lkk2003rty

  • since pr #4347 merged, function newBaseBuiltinFuncWithTp does not return error any more, plz update code and fix ci
  • plz correct the type inference of Flen of builtin function INTDIV
Member

zz-jason commented Aug 29, 2017

@lkk2003rty

  • since pr #4347 merged, function newBaseBuiltinFuncWithTp does not return error any more, plz update code and fix ci
  • plz correct the type inference of Flen of builtin function INTDIV

@zz-jason zz-jason referenced this pull request Aug 29, 2017

Closed

rewrite builtin arithmetic functions #3851

6 of 6 tasks complete
@lkk2003rty

This comment has been minimized.

Show comment
Hide comment
@lkk2003rty

lkk2003rty Aug 29, 2017

Contributor

I just fix Flen as mysql_int_div. But it seems that TiDB is a little different from MySQL, please check it.

Contributor

lkk2003rty commented Aug 29, 2017

I just fix Flen as mysql_int_div. But it seems that TiDB is a little different from MySQL, please check it.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 29, 2017

Member

what problem do you meet ? give us some examples so we can help you

Member

zz-jason commented Aug 29, 2017

what problem do you meet ? give us some examples so we can help you

@lkk2003rty

This comment has been minimized.

Show comment
Hide comment
@lkk2003rty

lkk2003rty Aug 29, 2017

Contributor

In MySQL‘s source code it use args[0]->max_char_length() minus args[0]->decimals to compute the length of return type. Now, I use the Flen of args[0] as args[0]->max_char_length(). I'm not sure it is the right way to do it.

Contributor

lkk2003rty commented Aug 29, 2017

In MySQL‘s source code it use args[0]->max_char_length() minus args[0]->decimals to compute the length of return type. Now, I use the Flen of args[0] as args[0]->max_char_length(). I'm not sure it is the right way to do it.

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 30, 2017

Member

@lkk2003rty for INTDIV, which only returns integer result, you can just set Flen and Decimal of return type to be mysql.MaxIntWidth and 0 respectively, no need to be compatible with mysql

Member

zz-jason commented Aug 30, 2017

@lkk2003rty for INTDIV, which only returns integer result, you can just set Flen and Decimal of return type to be mysql.MaxIntWidth and 0 respectively, no need to be compatible with mysql

return 0, isNull, errors.Trace(err)
}
if b == 0 {

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

can we first evaluate b and check whether it is zero, if so, we do not need to calculate a any more.

@zz-jason

zz-jason Aug 31, 2017

Member

can we first evaluate b and check whether it is zero, if so, we do not need to calculate a any more.

This comment has been minimized.

@lkk2003rty

lkk2003rty Aug 31, 2017

Contributor

good idea, I'll fix it

@lkk2003rty

lkk2003rty Aug 31, 2017

Contributor

good idea, I'll fix it

This comment has been minimized.

@zz-jason

zz-jason Sep 4, 2017

Member

in mysql:

MySQL > select 1 div 0;
+---------+
| 1 div 0 |
+---------+
|    NULL |
+---------+
1 row in set, 1 warning (0.00 sec)

MySQL > show warnings;
+---------+------+---------------+
| Level   | Code | Message       |
+---------+------+---------------+
| Warning | 1365 | Division by 0 |
+---------+------+---------------+
1 row in set (0.00 sec)

we should handle the divided by zero error, see #4373 for more detail

@zz-jason

zz-jason Sep 4, 2017

Member

in mysql:

MySQL > select 1 div 0;
+---------+
| 1 div 0 |
+---------+
|    NULL |
+---------+
1 row in set, 1 warning (0.00 sec)

MySQL > show warnings;
+---------+------+---------------+
| Level   | Code | Message       |
+---------+------+---------------+
| Warning | 1365 | Division by 0 |
+---------+------+---------------+
1 row in set (0.00 sec)

we should handle the divided by zero error, see #4373 for more detail

return 0, isNull, errors.Trace(err)
}
if b == 0 {

This comment has been minimized.

@zz-jason

zz-jason Aug 31, 2017

Member

ditto

@zz-jason

This comment has been minimized.

@lkk2003rty

lkk2003rty Aug 31, 2017

Contributor

fix it, PTAL

@lkk2003rty

lkk2003rty Aug 31, 2017

Contributor

fix it, PTAL

lkk2003rty and others added some commits Aug 31, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 31, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Aug 31, 2017

to #4080

@zz-jason zz-jason changed the title from expression: rewrite builtin function: IntDiv to expression: rewrite builtin function: DIV Aug 31, 2017

@zz-jason zz-jason changed the title from expression: rewrite builtin function: DIV to expression: rewrite builtin function: INTDIV Aug 31, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Aug 31, 2017

Member

in #4393, I make the following name changing:

find . -name "*.go" | xargs sed -i "s/\<isDeterministic\>/canBeFolded/g"
find . -name "*.go" | xargs sed -i "s/\<deterministic\>/foldable/g"

please update your code and do the same change to make build success

Member

zz-jason commented Aug 31, 2017

in #4393, I make the following name changing:

find . -name "*.go" | xargs sed -i "s/\<isDeterministic\>/canBeFolded/g"
find . -name "*.go" | xargs sed -i "s/\<deterministic\>/foldable/g"

please update your code and do the same change to make build success

@lkk2003rty

This comment has been minimized.

Show comment
Hide comment
@lkk2003rty

lkk2003rty Aug 31, 2017

Contributor

update for #4393 done

Contributor

lkk2003rty commented Aug 31, 2017

update for #4393 done

Show outdated Hide outdated expression/builtin_arithmetic_test.go
Show outdated Hide outdated expression/builtin_arithmetic.go
Show outdated Hide outdated expression/builtin_arithmetic.go
Show outdated Hide outdated expression/integration_test.go
Show outdated Hide outdated expression/builtin_arithmetic.go
Show outdated Hide outdated expression/builtin_arithmetic.go
Show outdated Hide outdated expression/builtin_arithmetic.go
@lkk2003rty

This comment has been minimized.

Show comment
Hide comment
@lkk2003rty

lkk2003rty Sep 1, 2017

Contributor

@XuHuaiyu PTAL again

Contributor

lkk2003rty commented Sep 1, 2017

@XuHuaiyu PTAL again

@sre-bot

This comment has been minimized.

Show comment
Hide comment
@sre-bot

sre-bot Sep 1, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

sre-bot commented Sep 1, 2017

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 1, 2017

Member

/ok-to-test

Member

iamxy commented Sep 1, 2017

/ok-to-test

zz-jason and others added some commits Sep 4, 2017

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason
Member

zz-jason commented Sep 5, 2017

@XuHuaiyu PTAL

@XuHuaiyu

This comment has been minimized.

Show comment
Hide comment
@XuHuaiyu

XuHuaiyu Sep 5, 2017

Contributor

rest LGTM

Contributor

XuHuaiyu commented Sep 5, 2017

rest LGTM

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 5, 2017

Member

/run-all-test

Member

zz-jason commented Sep 5, 2017

/run-all-test

@zz-jason

This comment has been minimized.

Show comment
Hide comment
@zz-jason

zz-jason Sep 5, 2017

Member

/run-all-test

Member

zz-jason commented Sep 5, 2017

/run-all-test

@iamxy

This comment has been minimized.

Show comment
Hide comment
@iamxy

iamxy Sep 5, 2017

Member

@zz-jason
You don't have to run all tests every time, just need to re run the failed test is ok. like this:
/run-integration-common-test
/run-integration-ddl-test

Member

iamxy commented Sep 5, 2017

@zz-jason
You don't have to run all tests every time, just need to re run the failed test is ok. like this:
/run-integration-common-test
/run-integration-ddl-test

@XuHuaiyu XuHuaiyu added status/LGT2 and removed status/LGT1 labels Sep 5, 2017

@XuHuaiyu

LGTM

@XuHuaiyu XuHuaiyu merged commit 92bea5a into pingcap:master Sep 5, 2017

10 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci-tidb/build Jenkins job succeeded.
Details
jenkins-ci-tidb/common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
jenkins-ci-tidb/mybatis-test Jenkins job succeeded.
Details
jenkins-ci-tidb/sqllogic-test Jenkins job succeeded.
Details
jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details

mccxj added a commit to mccxj/tidb that referenced this pull request Sep 7, 2017

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