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

Separate signatures for PlusInt #13973

Open
breezewish opened this issue Dec 9, 2019 · 4 comments · May be fixed by #22163 or #22026
Open

Separate signatures for PlusInt #13973

breezewish opened this issue Dec 9, 2019 · 4 comments · May be fixed by #22163 or #22026
Labels
component/expression type/enhancement The issue or PR belongs to an enhancement.

Comments

@breezewish
Copy link
Member

Description

Currently for the PlusInt built-in function, TiDB executes different logic according to whether LHS and RHS is signed or unsigned:

  • func (b *builtinArithmeticPlusIntSig) plusUU
  • func (b *builtinArithmeticPlusIntSig) plusUS
  • func (b *builtinArithmeticPlusIntSig) plusSU
  • func (b *builtinArithmeticPlusIntSig) plusSS

The actual logic is determined in func (b *builtinArithmeticPlusIntSig) vecEvalInt, i.e. re-checked every 1024 rows.

Meanwhile, at TiKV side, TiKV also does the same thing in the function signature mapper:

fn plus_mapper(lhs_is_unsigned: bool, rhs_is_unsigned: bool) -> RpnFnMeta {
    match (lhs_is_unsigned, rhs_is_unsigned) {
        (false, false) => arithmetic_fn_meta::<IntIntPlus>(),
        (false, true) => arithmetic_fn_meta::<IntUintPlus>(),
        (true, false) => arithmetic_fn_meta::<UintIntPlus>(),
        (true, true) => arithmetic_fn_meta::<UintUintPlus>(),
    }
}

A better way is to separate UU/US/SU/SS into different signatures at the planning stage instead of execution stage, i.e. func (c *arithmeticPlusFunctionClass) getFunction. A separation in getFunction can benefit both TiDB execution and TiKV execution, which can make the code simple, keep TiKV pure (no function deduction logic) and make execution slightly faster.

To finish this task, you need to:

  1. Modify tipb to introduce 4 new signatures, like:

    • builtinArithmeticPlusIntUnsignedUnsigned
    • builtinArithmeticPlusIntUnsignedSigned
    • ...

    (for compatibility consideration, builtinArithmeticPlusInt should be kept in tipb so that TiKV can still support old TiDB)

  2. Modify the func (c *arithmeticPlusFunctionClass) getFunction to introduce dispatch logic, dispatching to the new 4 signatures according to the operator's unsigned flag.

  3. Implement the new 4 function signature (and their vectorized version) by copying and refining code from the original function signature.

Optionally, you may consider modifying TiKV code base as well, to support these 4 newly added signatures. This will be in a separated task in TiKV side.

Difficulty

  • Easy

Score

  • 100

Mentor(s)

Recommended Skills

  • Go programming
@ChiaoTeo
Copy link
Contributor

I am trying fix it

@ChiaoTeo
Copy link
Contributor

/pick-up-challenge

@sre-bot
Copy link
Contributor

sre-bot commented Dec 18, 2019

@zhaoqian888 pick up issue success

@you06 you06 changed the title PCP: Separate signatures for PlusInt Separate signatures for PlusInt Feb 29, 2020
@Tjianke
Copy link
Contributor

Tjianke commented Dec 25, 2020

/pick-up-challenge #22026

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
5 participants