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: separated arithmeticMinusIntSig #22163

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

Conversation

Tjianke
Copy link
Contributor

@Tjianke Tjianke commented Jan 4, 2021

What problem does this PR solve?

Issue Number:

Problem Summary:

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

What is changed and how it works?

What's Changed:

  • Separated and vectorized old func signature by the signedness of args and sql_mode(NO_UNSIGNED_SUBTRACTION)
  • Updated tipb dependency to introduce new func signatures
  • Rewrote unit test for arithmeticMinusInt, making it much more easier to add unit tests

How it Works:

  • Rewrote dispatch logic in func (c *arithmeticMinusFunctionClass) getFunction(ctx sessionctx.Context, args), introduced 7 new func sig.
  • Overflow logic is reused from original vectorized func, except for cases where bugs were found(fixing bugs in expression: fix bugs in builtinfunction ArithmeticMinusInt logic #22426 ).
    • builtinArithmeticMinusIntSignedUnsignedSig
      • Overflow detection loigc: a < 0 || uint64(a) < uint64(b)
    • builtinArithmeticMinusIntUnsignedUnsignedSig
      • Overflow detection logic: uint64(a) > uint64(b)
    • builtinArithmeticMinusIntUnsignedSignedSig
      • Overflow detection logic: (b < 0 && uint64(a) > math.MaxUint64-uint64(-b)) || (b >= 0 && uint64(a) < uint64(b))
    • builtinArithmeticMinusIntSignedSignedSig*(works whether NO_UNSIGNED_SUBTRACTION mode is set or not)
      • Overflow detection logic: (lh >= 0 && rh == math.MinInt64) || (lh > 0 && -rh > math.MaxInt64-lh) || (lh < 0 && -rh < math.MinInt64-lh)
    • builtinArithmeticMinusIntForcedUnsignedUnsignedSig
      • Overflow detection logic: (a > 0 && -b > math.MaxInt64-a) || (a < 0 && -b < math.MinInt64-a)
    • builtinArithmeticMinusIntForcedSignedUnsignedSig
      • Overflow detection logic: (a > 0 && -b > math.MaxInt64-a) || (a < 0 && -b < math.MinInt64-a)
    • builtinArithmeticMinusIntForcedUnsignedSignedSig
      • Overflow detection logic: (a > 0 && -b > math.MaxInt64-a) || (a < 0 && -b < math.MinInt64-a)

Check List

Tests

  • Unit test

Release note

  • separated signatures for builtin MinusIntSig

@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 4, 2021
@Tjianke Tjianke marked this pull request as ready for review January 13, 2021 06:12
@Tjianke Tjianke requested a review from a team as a code owner January 13, 2021 06:12
@Tjianke Tjianke requested review from XuHuaiyu and removed request for a team January 13, 2021 06:12
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 13, 2021

/run-all-tests

@Tjianke Tjianke marked this pull request as draft January 13, 2021 16:03
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 14, 2021

/run-all-tests

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 14, 2021

/run-tics-test

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 14, 2021

Not sure what's wrong in tics-test, it suggets ERROR 1105 (HY000) at line 1: [FLASH:Coprocessor:Unimplemented] Unspecified is not supported.

/cc @breeswish @XuHuaiyu could you please take a look at this, btw is tics-test available to run in local dev environment?

@Tjianke Tjianke marked this pull request as ready for review January 14, 2021 03:44
@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 15, 2021

/run-all-tests

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 18, 2021

Depends on TiFlash to implement push down func

@breezewish
Copy link
Member

Thanks for the remind. Let's wait TiFlash to implement these separated signatures first.

@breezewish
Copy link
Member

Hi, considering that this PR mainly contains two parts:

  1. Logic separation, for performance improvements (this need to wait TiFlash)
  2. Bug fix (this need to be cherry picked to release branches)

Could you first open a PR to fix bugs? I think this can be merged pretty quickly, since it doesn't need to wait TiFlash.

@Tjianke
Copy link
Contributor Author

Tjianke commented Jan 18, 2021

Hi, considering that this PR mainly contains two parts:

  1. Logic separation, for performance improvements (this need to wait TiFlash)
  2. Bug fix (this need to be cherry picked to release branches)

Could you first open a PR to fix bugs? I think this can be merged pretty quickly, since it doesn't need to wait TiFlash.

Here's the bug-fixing PR(#22426).

@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 Feb 23, 2021
@guo-shaoge guo-shaoge removed the request for review from XuHuaiyu June 10, 2021 03:15
@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

Attention: Patch coverage is 69.52790% with 71 lines in your changes missing coverage. Please review.

Project coverage is 77.0910%. Comparing base (42edd7a) to head (a1c126a).
Report is 13001 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #22163        +/-   ##
================================================
- Coverage   77.1165%   77.0910%   -0.0255%     
================================================
  Files           573        573                
  Lines        146429     146598       +169     
================================================
+ Hits         112921     113014        +93     
- Misses        23347      23391        +44     
- Partials      10161      10193        +32     

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separate signatures for PlusInt
4 participants