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: separated arithmeticIntDivideSig #22374

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

expression: separated arithmeticIntDivideSig #22374

wants to merge 23 commits into from
+284 −64

Conversation

Tjianke
Copy link
Contributor

@Tjianke Tjianke commented Jan 13, 2021 •
edited
Loading

What problem does this PR solve?

Issue Number: closes #13979

Problem Summary:

  • Separating ArithmeticDivideIntSig at planning stage will be more efficient then current status

What is changed and how it works?

Benchmark result:

name                                                                                  old time/op  new time/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-VecBuiltinFunc-8     13.5µs ± 3%  12.3µs ±28%   -8.97%  (p=0.000 n=19+80)
VectorizedBuiltinArithmeticFunc/builtinArithmeticIntDivideIntSig-NonVecBuiltinFunc-8  41.5µs ± 3%  35.3µs ±12%  -14.88%  (p=0.000 n=20+78)

macIntDivOld.txt
macIntDivNew.txt

What's Changed:

  • Impl vectorized, separated func sig from original DivideIntSig, separated DivideIntSig into four signatures by sign/unsign of arguments

  • Updated tipb dependency to introduce new func signatures

How it Works:

Check List

Tests

  • Unit test

Release note

  • separated signatures for builtin DivideIntSig

@ti-srebot ti-srebot added the first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. label Jan 13, 2021
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 13, 2021

/run-all-tests

1 similar comment
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 13, 2021

/run-all-tests

@Tjianke Tjianke marked this pull request as ready for review January 13, 2021 02:07
@Tjianke Tjianke requested a review from a team as a code owner January 13, 2021 02:07
@Tjianke Tjianke requested review from qw4990 and removed request for a team January 13, 2021 02:07
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 13, 2021

/cc @breeswish @XuHuaiyu PTAL, thanks~

@breezewish
Copy link
Member

/cc @wshwsh12 @qw4990

PTAL, thanks!

@windtalker windtalker mentioned this pull request Jan 27, 2021
Merged
@Tjianke
Copy link
Contributor Author

Tjianke commented Feb 3, 2021

@ZiheLiu PTAL, PR involves code you contributed, thanks~

pingyu
pingyu suggested changes Feb 6, 2021
View reviewed changes
Copy link
Contributor

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

Suggest add test cases for "Signed-Unsigned" & "Unsigned-Signed" in TestArithmeticDivide. Rest LGTM.

@Tjianke Tjianke force-pushed the separateDivideInt branch 2 times, most recently from 6527613 to a740811 Compare February 6, 2021 15:31
pingyu
pingyu reviewed Feb 7, 2021
View reviewed changes
expression/integration_test.go Outdated
@@ -3507,6 +3507,25 @@ func (s *testIntegrationSuite) TestArithmeticBuiltin(c *C) {
c.Assert(terror.ErrorEqual(err, types.ErrOverflow), IsTrue)
c.Assert(rs.Close(), IsNil)

//IntDiv overflow test
Copy link
Contributor

Choose a reason for hiding this comment

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

@Tjianke
Cover only two of four overflow cases here looks a little strange to me.

In my previous comment I suggested to add test cases for "Signed-Unsigned" & "Unsigned-Signed" only because the other two (UU and SS) exists in (*testEvaluatorSuite) TestArithmeticDivide().

I think place US and SU in (*testEvaluatorSuite) TestArithmeticDivide() would be easier, and would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

while adding tests in TestArithmeticDivide, I found out another issue #22759, root cause is lying here

pingyu
pingyu reviewed Feb 9, 2021
View reviewed changes
expression/builtin_arithmetic_test.go Outdated
@@ -360,6 +360,14 @@ func (s *testEvaluatorSuite) TestArithmeticDivide(c *C) {
args: []interface{}{nil, nil},
expect: nil,
},
{
args: []interface{}{uint64(3), -3},
Copy link
Contributor

Choose a reason for hiding this comment

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

@Tjianke
It seems not working as expected. err returned by evalBuiltinFunc in line 389 should not be IsNil when overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests under TestArithmeticIntDivide , which covers cases of not nil err. Also switch case seems wrong in TestArithmeticDivide , as ast.Div applies only for Real and Decimal type.

pingyu
pingyu reviewed Feb 10, 2021
View reviewed changes
expression/builtin_arithmetic_test.go
case *builtinArithmeticDivideRealSig:
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_DivideReal)
case *builtinArithmeticDivideDecimalSig:
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_DivideDecimal)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest add "default" branch and raise failure, to avoid similar error in the future.

pingyu
pingyu reviewed Feb 10, 2021
View reviewed changes
expression/builtin_arithmetic_test.go
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_IntDivideIntUnsignedSigned)
case *builtinArithmeticIntDivideIntUnsignedUnsignedSig:
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_IntDivideIntUnsignedUnsigned)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

pingyu
pingyu suggested changes Feb 10, 2021
View reviewed changes
Copy link
Contributor

@pingyu pingyu left a comment

Choose a reason for hiding this comment

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

It would be more common to use c.Fatal in unit test.

expression/builtin_arithmetic_test.go Outdated
@@ -371,6 +371,8 @@ func (s *testEvaluatorSuite) TestArithmeticDivide(c *C) {
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_DivideReal)
case *builtinArithmeticDivideDecimalSig:
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_DivideDecimal)
default:
panic("Wrong type of signature tested")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
panic("Wrong type of signature tested")
c.Fatal("Wrong type of signature tested")

expression/builtin_arithmetic_test.go Outdated
case *builtinArithmeticIntDivideDecimalSig:
c.Assert(sig.PbCode(), Equals, tipb.ScalarFuncSig_IntDivideDecimal)
default:
panic("Wrong type of signature tested")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
panic("Wrong type of signature tested")
c.Fatal("Wrong type of signature tested")

@pingyu
Copy link
Contributor

pingyu commented Feb 10, 2021

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 10, 2021
@ti-chi-bot ti-chi-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Feb 24, 2021
@ti-chi-bot
Copy link
Member

@Tjianke: PR needs rebase.

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 24, 2021
@qw4990 qw4990 requested review from a team and XuHuaiyu and removed request for qw4990 and a team June 21, 2021 03:34
@XuHuaiyu
Copy link
Contributor

The DIV function can be pushed down to the tikv and tiflash.
Can tikv and tiflash rebuild the DIV function with these new tipb_codes?

@XuHuaiyu XuHuaiyu removed their request for review June 30, 2021 03:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pingyu pingyu pingyu requested changes

Assignees
No one assigned
Labels
component/expression first-time-contributor Indicates that the PR was contributed by an external member and is a first-time contributor. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Separate signatures for IntDivideInt
7 participants
@Tjianke @breezewish @pingyu @ti-chi-bot @XuHuaiyu @tisonkun @ti-srebot