expression: speed up builtinRepeatSig by using MergeNulls

[js00070]

What problem does this PR solve?

I find a TODO in the code here #12014 (comment)

So I tried to introduce vectorized null-bitmap by using function MergeNulls to speed it up

What is changed and how it works?

I use MergeNulls in builtinInsertSig, and add a rules check in MergeNulls to avoid it be called wrongly

Check List

Tests

• Unit test

[codecov]

Codecov Report

Merging #12674 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12674   +/-   ##
===========================================
  Coverage   79.9727%   79.9727%           
===========================================
  Files           465        465           
  Lines        107319     107319           
===========================================
  Hits          85826      85826           
  Misses        15029      15029           
  Partials       6464       6464

[XuHuaiyu]

Thanks for your contribution.
It seems that MergeNulls is called wrongly in this PR.
Please check the comment of MergeNull.

It will be nice if you can add a check in MergeNulls for the rules that the called need to follow to avoid it be called wrongly again.

[js00070]

Thanks for your contribution.
It seems that MergeNulls is called wrongly in this PR.
Please check the comment of MergeNull.

It will be nice if you can add a check in MergeNulls for the rules that the called need to follow to avoid it be called wrongly again.

Thanks for reviewing my change of code.
I can see data stored in the result column is fixed-length type. Although I haven't figured out the reason why it should be fixed-length type yet, but I will try to add a check in MergeNulls first.

[js00070]

Thanks for your contribution.
It seems that MergeNulls is called wrongly in this PR.
Please check the comment of MergeNull.

It will be nice if you can add a check in MergeNulls for the rules that the called need to follow to avoid it be called wrongly again.

It seems that there is a wrongly called MergeNulls in the master branch, at here

tidb/expression/builtin_string_vec.go

Line 365 in 9dacf84

result.MergeNulls(buf, buf1)

result is a int type column, buf and buf1 are string type columns. It seems to violate the requirements of MergeNulls

[XuHuaiyu]

@js00070
MergeNulls ONLY needs the result column to be fixed-length type.
That's because we use AppendXXX to write a new value to a var-length type column, and AppendXXX will also append a null-value into nullBitmap which is wrong.

[js00070]

@js00070
MergeNulls ONLY needs the result column to be fixed-length type.
That's because we use AppendXXX to write a new value to a var-length type column, and AppendXXX will also append a null-value into nullBitmap which is wrong.

Thanks! I totally understand now!

[SunRunAway]

Could you provide a benchmark result.
Try to use https://godoc.org/golang.org/x/perf/cmd/benchstat.

Beside, could you help to remove that TODO you mentioned before?

[SunRunAway]

Suggested change

	if len(c.offsets) != 0 {
	if !c.isFixed() {

[js00070]

Could you provide a benchmark result.
Try to use https://godoc.org/golang.org/x/perf/cmd/benchstat.

Beside, could you help to remove that TODO you mentioned before?

I run the bench test and use benchstat to compare the performance between using MergeNulls and not using MergeNulls, the old.txt is the benchtest result without using MergeNulls, and the new.txt is the benchtest result with using MergeNulls.

here is my benchtest cmd:

go test -v -benchmem -bench=BenchmarkVectorizedBuiltinArithmeticFunc -count=20 -run=BenchmarkVectorizedBuiltinArithmeticFunc | tee old.txt

go test -v -benchmem -bench=BenchmarkVectorizedBuiltinArithmeticFunc -count=20 -run=BenchmarkVectorizedBuiltinArithmeticFunc | tee new.txt

here is my benchstat result:

$ benchstat old.txt new.txt
name                                                                            old time/op    new time/op    delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticMinusRealSig-VecBuiltinFunc-4    4.30µs ± 3%    3.96µs ± 2%  -7.72%  (p=0.000 n=18+19)

name                                                                            old alloc/op   new alloc/op   delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticMinusRealSig-VecBuiltinFunc-4     0.00B          0.00B         ~     (all equal)

name                                                                            old allocs/op  new allocs/op  delta
VectorizedBuiltinArithmeticFunc/builtinArithmeticMinusRealSig-VecBuiltinFunc-4      0.00           0.00         ~     (all equal)

here is my bench test code with no MergeNulls:

func (b *builtinArithmeticMinusRealSig) vecEvalReal(input *chunk.Chunk, result *chunk.Column) error {
	if err := b.args[0].VecEvalReal(b.ctx, input, result); err != nil {
		return err
	}
	n := input.NumRows()
	buf, err := b.bufAllocator.get(types.ETReal, n)
	if err != nil {
		return err
	}
	defer b.bufAllocator.put(buf)
	if err := b.args[1].VecEvalReal(b.ctx, input, buf); err != nil {
		return err
	}

	// result.MergeNulls(buf)
	x := result.Float64s()
	y := buf.Float64s()
	for i := 0; i < n; i++ {
		if result.IsNull(i) || buf.IsNull(i) {
			continue
		}
		if (x[i] > 0 && -y[i] > math.MaxFloat64-x[i]) || (x[i] < 0 && -y[i] < -math.MaxFloat64-x[i]) {
			return types.ErrOverflow.GenWithStackByArgs("DOUBLE", fmt.Sprintf("(%s - %s)", b.args[0].String(), b.args[1].String()))
		}
		x[i] = x[i] - y[i]
	}
	return nil
}

[SunRunAway]

LGTM.

[zyxbest]

/build

[zyxbest]

/run-all-tests

[XuHuaiyu]

LGTM

[sre-bot]

/run-all-tests