expression, plan: support builtin aggregation function 'bit_or' #5145

spongedu · 2017-11-18T07:15:20Z

support builtin aggregation function 'bit_or'

sre-bot · 2017-11-18T07:15:22Z

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

shenli · 2017-11-18T10:40:07Z

@spongedu Thanks!
Please fix CI.

zz-jason · 2017-11-20T02:39:37Z

/run-all-tests

zz-jason · 2017-11-20T03:11:03Z

/run-sqllogic-test

zz-jason

LGTM

shenli · 2017-11-22T16:08:50Z

store/tikv/coprocessor.go

@@ -78,7 +78,7 @@ func (c *CopClient) supportExpr(exprType tipb.ExprType) bool {
 	case tipb.ExprType_Case, tipb.ExprType_If, tipb.ExprType_IfNull, tipb.ExprType_Coalesce:
 		return true
 	// aggregate functions.
-	case tipb.ExprType_Count, tipb.ExprType_First, tipb.ExprType_Max, tipb.ExprType_Min, tipb.ExprType_Sum, tipb.ExprType_Avg:
+	case tipb.ExprType_Count, tipb.ExprType_First, tipb.ExprType_Max, tipb.ExprType_Min, tipb.ExprType_Sum, tipb.ExprType_Avg, tipb.ExprType_Agg_BitOr:


TiKV doesn't support ExprType_Agg_BitOr.

@shenli how about this:

Step1: disable FinalMode for BIT_OR so that it's not calculated in TIKV.

Step2: take another pr in TiKV to support BIT_OR IN TIKV.

Step3: re-enable FinalMode for BIT_OR

If the agg is bit_or, we return c.store.mock so as to push it to mock tikv instead of real tikv.

@hanfei1991

you mean that BIT_OR is never push down to TiKV, even in real clustered environment?

This pr seems works ok with mock-tikv

I think it's still necessary to support BIT_OR in TiKV anyway

what do you think

After tikv has supported this aggregate function, we can remove this check. This is only a protection mechanism.

@hanfei1991 PTAL. I disabled FinalMode for aggregation BIT_OR. This check seems can be removed

…t's support in tikv.

Conflicts: ast/functions.go expression/aggregation/agg_to_pb.go expression/aggregation/aggregation.go expression/integration_test.go plan/physical_plans.go

zz-jason · 2017-12-07T02:39:11Z

/run-all-tests

zz-jason · 2017-12-07T04:41:45Z

/run-integration-common-test

zimulala · 2017-12-08T03:41:42Z

expression/aggregation/bit_or.go

+	ft := types.NewFieldType(mysql.TypeLonglong)
+	ft.Flen = 21
+	types.SetBinChsClnFlag(ft)
+	ft.Flag |= mysql.UnsignedFlag


Do we need to add the flag of not null?

mysql> select bit_or(b) from t; Field 1: `bit_or(b)` Catalog: `def` Database: `` Table: `` Org_table: `` Type: LONGLONG Collation: binary (63) Length: 21 Max_length: 1 Decimals: 0 Flags: NOT_NULL UNSIGNED BINARY NUM

@zimulala yes. The same for BIT_AND and BIT_XOR.

zimulala · 2017-12-08T03:53:44Z

expression/aggregation/bit_or.go

+// GetResult implements Aggregation interface.
+func (bf *bitOrFunction) GetResult(ctx *AggEvaluateContext) (d types.Datum) {
+	d.SetUint64(ctx.Value.GetUint64())
+	return d


Why not return "ctx.Value" directly?

By cursoriness.... Will fix

zimulala · 2017-12-08T03:56:03Z

expression/aggregation/bit_or.go

+		if ctx.Value.IsNull() {
+			ctx.Value.SetUint64(0)
+		}
+		v := row.GetUint64(0)


Why here is 0?

@zimulala you mean the 0 in line 83? what's wrong with it ?

Yep. I don't know why the column index is 0.

Besides, can we handle the operation of Update use this same way in different modes?

@zimulala yes, for bit_or, bit_and, bit_xor, FinalMode and Complete mode seem act the same , I'll refine here.

spongedu · 2017-12-08T09:55:30Z

@zimulala merge FinalMode and CompleteMode in Update. PTAL

shenli

Rest LGTM

shenli · 2017-12-08T10:09:19Z

expression/aggregation/bit_or.go

+
+// CalculateDefaultValue implements Aggregation interface.
+func (bf *bitOrFunction) CalculateDefaultValue(schema *expression.Schema, ctx context.Context) (d types.Datum, valid bool) {
+	arg := bf.Args[0]


Should we also check len(bf.Args[0]) as line 62?

@shenli actually I think the check in line 62 should be removed as well. There's no need check argument length everytime Update is called. Parser would fail if argument number is wrong unless something is wrong in the framework. Or we can do this check in expression/aggregation/aggregation.go

zimulala · 2017-12-08T10:25:38Z

LGTM

tiancaiamao · 2017-12-19T10:53:52Z

/run-all-tests

tiancaiamao · 2017-12-19T11:20:50Z

/run-sqllogic-test

tiancaiamao · 2017-12-19T11:42:51Z

/run-sqllogic-test

expression, plan: support builtin aggregation function 'bit_or'

078594b

spongedu added 2 commits November 18, 2017 15:30

Fix build fail

8247488

Fix build fail

fc900f3

spongedu and others added 2 commits November 18, 2017 19:52

Fix build fail

b16d6de

Merge branch 'master' into agg_bit_or

13bb70e

zz-jason reviewed Nov 20, 2017

View reviewed changes

zz-jason added contribution This PR is from a community contributor. status/LGT1 Indicates that a PR has LGTM 1. labels Nov 20, 2017

zz-jason added this to the 1.1 milestone Nov 20, 2017

spongedu and others added 9 commits November 20, 2017 15:40

Fix spell error

af37448

Merge remote-tracking branch 'upstream/master' into agg_bit_or

ba05f31

Merge branch 'agg_bit_or' of github.com:spongedu/tidb into agg_bit_or

6291f6c

Merge branch 'master' into agg_bit_or

effde61

Merge branch 'master' into agg_bit_or

f9fe388

Fxi build fail

7368f15

Merge remote-tracking branch 'upstream/master' into agg_bit_or

05ac14b

Fix build fail due to code merge

85be05d

Merge branch 'master' into agg_bit_or

e9d2c47

shenli reviewed Nov 22, 2017

View reviewed changes

spongedu added 4 commits November 23, 2017 09:56

Merge branch 'master' into agg_bit_or

901d19c

Merge branch 'master' into agg_bit_or

81e42ed

Disable FinalMode for builtin aggregation function bit_or until i…

7b73eec

…t's support in tikv.

disable 'bit_or' in store/tikv/coprocessor.go

5a6d9bb

coocood added the conflicting label Nov 24, 2017

Merge remote-tracking branch 'upstream/master' into agg_bit_or

2636510

Conflicts: ast/functions.go expression/aggregation/agg_to_pb.go expression/aggregation/aggregation.go expression/integration_test.go plan/physical_plans.go

zz-jason removed the conflicting label Nov 27, 2017

spongedu and others added 5 commits November 29, 2017 16:48

Merge branch 'master' into agg_bit_or

3acf050

Merge branch 'master' into agg_bit_or

38a4397

Merge branch 'master' into agg_bit_or

35428b2

Merge remote-tracking branch 'upstream/master' into agg_bit_or

ac4bd8f

Merge branch 'master' into agg_bit_or

a908809

zz-jason mentioned this pull request Dec 7, 2017

unsupported aggregate function caused a "nil pointer dereference" panic #5339

Closed

zimulala reviewed Dec 8, 2017

View reviewed changes

spongedu added 2 commits December 8, 2017 15:23

Code format refine

459865e

Merge remote-tracking branch 'upstream/master' into agg_bit_or

98b22b2

spongedu force-pushed the agg_bit_or branch from 8a9175c to 9ea0ec7 Compare December 8, 2017 07:28

Code format refine

e089f04

spongedu force-pushed the agg_bit_or branch from 9ea0ec7 to e089f04 Compare December 8, 2017 07:35

Merge logic for 'FinalMode' and 'CompleteMode' in 'Update'

94c9203

shenli reviewed Dec 8, 2017

View reviewed changes

spongedu and others added 3 commits December 8, 2017 19:01

Remove arg length check in 'Update'

73c1379

Merge branch 'master' into agg_bit_or

2777799

Merge branch 'master' into agg_bit_or

4377eb1

zz-jason approved these changes Dec 19, 2017

View reviewed changes

zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 19, 2017

zz-jason and others added 2 commits December 19, 2017 16:18

Merge branch 'master' into agg_bit_or

57adba8

Merge branch 'master' into agg_bit_or

f040235

tiancaiamao merged commit 6b6afb6 into pingcap:master Dec 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expression, plan: support builtin aggregation function 'bit_or' #5145

expression, plan: support builtin aggregation function 'bit_or' #5145

spongedu commented Nov 18, 2017

sre-bot commented Nov 18, 2017

shenli commented Nov 18, 2017

zz-jason commented Nov 20, 2017

zz-jason commented Nov 20, 2017

zz-jason left a comment

shenli Nov 22, 2017

spongedu Nov 23, 2017

hanfei1991 Nov 23, 2017

spongedu Nov 23, 2017 •

edited

Loading

hanfei1991 Nov 23, 2017

spongedu Nov 23, 2017

spongedu Nov 23, 2017

zz-jason commented Dec 7, 2017

zz-jason commented Dec 7, 2017

zimulala Dec 8, 2017

spongedu Dec 8, 2017

zimulala Dec 8, 2017

spongedu Dec 8, 2017

zimulala Dec 8, 2017

spongedu Dec 8, 2017

zimulala Dec 8, 2017

zimulala Dec 8, 2017 •

edited

Loading

spongedu Dec 8, 2017

spongedu commented Dec 8, 2017

shenli left a comment

shenli Dec 8, 2017

spongedu Dec 8, 2017

zimulala commented Dec 8, 2017

tiancaiamao commented Dec 19, 2017

tiancaiamao commented Dec 19, 2017

tiancaiamao commented Dec 19, 2017

expression, plan: support builtin aggregation function 'bit_or' #5145

expression, plan: support builtin aggregation function 'bit_or' #5145

Conversation

spongedu commented Nov 18, 2017

sre-bot commented Nov 18, 2017

shenli commented Nov 18, 2017

zz-jason commented Nov 20, 2017

zz-jason commented Nov 20, 2017

zz-jason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zz-jason commented Dec 7, 2017

zz-jason commented Dec 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spongedu commented Dec 8, 2017

shenli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zimulala commented Dec 8, 2017

tiancaiamao commented Dec 19, 2017

tiancaiamao commented Dec 19, 2017

tiancaiamao commented Dec 19, 2017

spongedu Nov 23, 2017 •

edited

Loading

zimulala Dec 8, 2017 •

edited

Loading