expression: Short cut expr vec bug

[fzhedu]

What problem does this PR solve?

Issue Number: close #17725

Problem Summary: fallback vectorization to scalar execution when error/warnings happen.

What is changed and how it works?

Proposal: xxx

What's Changed:

How it Works:

Related changes

• Need to cherry-pick to the release branch

Check List

Tests

• Integration test

Release note

• solve vectorization bug from and/or/COALESCE due to short cut

[zz-jason]

I think we can modify the vectorized function to:

• x OR y: if x[i] = 0, evaluate y[i], else skip y[i]
• x AND y: if x[i] = 1, evaluate y[i], else skip y[i]

[fzhedu]

I think we can modify the vectorized function to:

• x OR y: if x[i] = 0, evaluate y[i], else skip y[i]
• x AND y: if x[i] = 1, evaluate y[i], else skip y[i]

This would be more complex.

1. this way does not change the fact that vectorization should evaluate every arg.
   2.it indeed adds an additional loop to check x[i] == 0/1 ? before y[i], but not shortcut the execution like in the scalar execution.

[zz-jason]

I think we can modify the vectorized function to:

• x OR y: if x[i] = 0, evaluate y[i], else skip y[i]
• x AND y: if x[i] = 1, evaluate y[i], else skip y[i]

This would be more complex.

1. this way does not change the fact that vectorization should evaluate every arg.
   2.it indeed adds an additional loop to check x[i] == 0/1 ? before y[i], but not shortcut the execution like in the scalar execution.

We can firstly evaluate the x vector, then evaluate each scalar value on y[i] based on the result of x[i].

BTW, how other vectorized databases handles these kinds of expressions?

[fzhedu]

I think we can modify the vectorized function to:

• x OR y: if x[i] = 0, evaluate y[i], else skip y[i]
• x AND y: if x[i] = 1, evaluate y[i], else skip y[i]

This would be more complex.

1. this way does not change the fact that vectorization should evaluate every arg.
   2.it indeed adds an additional loop to check x[i] == 0/1 ? before y[i], but not shortcut the execution like in the scalar execution.

We can firstly evaluate the x vector, then evaluate each scalar value on y[i] based on the result of x[i].

BTW, how other vectorized databases handles these kinds of expressions?

In formal vectiorzation, each arg should be executed in vectorization.
y maybe other complex functions, not just a constant or a column, it is better to vectorize each arg. Once erros/warnings happen (rarely happen), falls back to the scalar execution.

[zz-jason]

to summarize, there are two methods in the above to handle short circuit for x AND y:

1. evaluate all the arguments to get vector x and y, if not all the values in x are 0, fallback to the row-based execution method.
2. evaluate the vector x firstly, conditionally evaluate y[i] based on the value of x[i].

IMO, the performance of the second method is better than the first method in most cases, where not all the values in the vector x are 0.

So I prefer the second method. It achieves the short circuit and the performance is not sacrificed much compared with the old vectorized method.

[fzhedu]

to summarize, there are two methods in the above to handle short circuit for x AND y:

1. evaluate all the arguments to get vector x and y, if not all the values in x are 0, fallback to the row-based execution method.
2. evaluate the vector x firstly, conditionally evaluate y[i] based on the value of x[i].

IMO, the performance of the second method is better than the first method in most cases, where not all the values in the vector x are 0.

So I prefer the second method. It achieves the short circuit and the performance is not sacrificed much compared with the old vectorized method.

It depends.
x and y if y = (a*b>C) such complex sub expressions, and x has certain 0s, taking the vectorization would better.

[zz-jason]

Would you like to construct some expression cases and do the performance benchmarks on both methods? It's hard to decide to use which one without any benchmark result.

[SunRunAway]

Would you like to construct some expression cases and do the performance benchmarks on both methods? It's hard to decide to use which one without any benchmark result.

Hi, @zz-jason
I suggest dividing this issue into 2 steps.

1. First, use method 1 to fix this bug as a workaround in this PR.
2. Then make a further plan to reconstruct the vectorized expression framework by method 2 in the future for performance improvement. cc @qw4990

[zz-jason]

I think method 1 and 2 are both workarounds, the difference is which workaround can have better performance. It's not hard to implement method 2 and do some benchmarks IMO.

[SunRunAway]

Method 2 needs to involve the selection vector into all the signatures of the vectorized expression framework. It may take plenty of time. It's better to create another issue and have a good design which may be a performance challenge program.
In the meantime, the existing bug should not be left in the release version so I think the workaround of this PR should be merged ASAP.

[zz-jason]

Method 2 needs to involve the selection vector into all the signatures of the vectorized expression framework.

No need to. We can combine the usage of evalXXX and VecEvalXXX in this workaround.

I think the workaround of this PR should be merged ASAP.

It's not that urgent to fix this issue since it only affects the warning messages.

[SunRunAway]

Method 2 needs to involve the selection vector into all the signatures of the vectorized expression framework.

No need to. We can combine the usage of evalXXX and VecEvalXXX in this workaround.

That's an even more complex workaround. And I think this could be a new performance issue since this improvement exiting before whenever this bug happens. Even more, I think it's still not worth involving another complex workaround since we have a better idea which uses the selection vector to do short circuit.

It's not that urgent to fix this issue since it only affects the warning messages.

DML queries treat it as an error.

mysql> update t set a=1 and 1/0;
ERROR 1365 (22012): Division by 0

[zz-jason]

If there is an error in the DML statements, the DML queries won't execute successfully, thus the data correctness and consistency can be guaranteed. So I think it's not that urgent.

Though both methods are workarounds, I think it's important to reduce the performance regression. I'm OK with method 1 if the benchmark result shows that method 1 brings less performance regression than method 2.

[SunRunAway]

LGTM

[zz-jason]

LGTM

[zz-jason]

/merge

[fzhedu]

/merge

[ti-srebot]

/run-all-tests

[ti-srebot]

@fzhedu merge failed.

[fzhedu]

/run-check_dev_2

[fzhedu]

/run-check_dev_2

[fzhedu]

/merge

[ti-srebot]

Your auto merge job has been accepted, waiting for:

• 19998

[ti-srebot]

/run-all-tests

[ti-srebot]

/run-all-tests

[ti-srebot]

@fzhedu merge failed.

[SunRunAway]

/merge

[ti-srebot]

/run-all-tests

[ti-srebot]

cherry pick to release-4.0 in PR #20092