*: move ast.NewValueExpr to standalone parser_driver package #7952

tiancaiamao · 2018-10-18T12:11:23Z

What problem does this PR solve?

Make the ast package get rid of the dependency of types.Datum
The first step for #7923

What is changed and how it works?

Make ValueExpr a interface, define a NewValueExpr function
Move the old ValueExpr to the parser_driver package, provide the NewValueExpr implementation
ParamMarkerExpr is also moved, it's similar to ValueExpr
Move RowToDatums from the ast package to statistics

The types package depends on a lot of thing, because types.Datum need Time, Decimal, Json and so on and use the util package.
To make the ast package get rid of types, the first step is to decompose the dependency of types.Datum in this commit.

Check List

Tests

No code

Side effects

Increased code complexity

@lysu @jackysp

Make the ast package get rid of the dependency of types.Datum

tiancaiamao · 2018-10-18T12:12:10Z

/run-all-tests

zz-jason · 2018-10-18T12:16:27Z

statistics/sample.go

@@ -186,3 +186,12 @@ func (s SampleBuilder) CollectColumnStats() ([]*SampleCollector, *SortedBuilder,
 		}
 	}
 }
+
+// RowToDatums converts row to datum slice.
+func RowToDatums(row chunk.Row, fields []*ast.ResultField) []types.Datum {


move it to the driver package?

I find it's introduced and used only by statistics, so I move it here.
What's your opinion @lamxTyler

I am fine with the change.

zz-jason · 2018-10-18T12:18:57Z

ast/base.go

-	return &en.Datum
-}
+// TexprNode is exported for parser driver.
+type TexprNode = exprNode


How about s/exprNode/ExprNode/ and exporting it?

There is already a ExprNode, rename exprNode will cause conflict.
ExprNode is an interface, and exprNode is the embed struct that implements ExprNode.

Use a type alias here could make changes minimal.

zz-jason · 2018-10-18T12:22:19Z

ast/flag.go

@@ -32,6 +32,9 @@ func (f *flagSetter) Enter(in Node) (Node, bool) {
 }

 func (f *flagSetter) Leave(in Node) (Node, bool) {
+	if x, ok := in.(ParamMarkerExpr); ok {


move this to the original place?

*ParamMarkerExpr is change from struct to interface, because the old struct definition depends on types.Datum

Go can't type assert on a interface:

type T interface { ... } switch x := in.(type) { case T: // compile error }

zz-jason · 2018-10-18T12:23:15Z

ast/misc_test.go

@@ -44,6 +44,7 @@ func (visitor1) Enter(in Node) (Node, bool) {
 }

 func (ts *testMiscSuite) TestMiscVisitorCover(c *C) {
+	valueExpr := NewValueExpr(42)


what does 42 mean?

42 is the "Answer to the Ultimate Question of Life, the Universe, and Everything" .

I remember @shenli asked this question before.
I like the number, through it's meaningless here.
This code improve test coverage.

zhexuany

LGTM

jackysp · 2018-10-19T08:57:31Z

planner/core/planbuilder.go

@@ -399,7 +400,8 @@ func (b *PlanBuilder) buildPrepare(x *ast.PrepareStmt) Plan {
 		Name: x.Name,
 	}
 	if x.SQLVar != nil {
-		p.SQLText, _ = x.SQLVar.GetValue().(string)
+		// TODO: Prepared statement from variable expression do not work as expected.
+		// p.SQLText, _ = x.SQLVar.GetValue().(string)


emmm... we passed all test cases but no one could cover it?

The code never runs to this branch and there is no test code cover here.
It doesn't not work (not caused by the change of this commit), maybe we can come back and fix here later, but not in this PR. @jackysp

jackysp

rest LGTM

jackysp · 2018-10-19T09:42:35Z

types/parser_driver/value_expr.go

@@ -0,0 +1,163 @@
+// Copyright 2015 PingCAP, Inc.


It's 2018 now.

jackysp

LGTM

bugfix fixed pingcap#7518 expression: MySQL compatible current_user function (pingcap#7801) plan: propagate constant over outer join (pingcap#7794) - extract `outerCol = const` from join conditions and filter conditions, substitute `outerCol` in join conditions with `const`; - extract `outerCol = innerCol` from join conditions, derive new join conditions based on this column equal condition and `outerCol` related expressions in join conditions and filter conditions; util/timeutil: fix data race caused by forgetting set stats lease to 0 (pingcap#7901) stats: handle ddl event for partition table (pingcap#7903) plan: implement Operand and Pattern of cascades planner. (pingcap#7910) planner: not convert to TableDual if empty range is derived from deferred constants (pingcap#7808) plan: move projEliminate behind aggEliminate (pingcap#7909) admin: fix admin check table bug of byte compare (pingcap#7887) * admin: remove reflect deepEqual stats: fix panic caused by empty histogram (pingcap#7912) plan: fix panic caused by empty schema of LogicalTableDual (pingcap#7906) * fix drop view if exist error (pingcap#7833) executor: refine `explain analyze` (pingcap#7888) executor: add an variable to compatible with MySQL insert for OGG (pingcap#7863) expression: maintain `DeferredExpr` in aggressive constant folding. (pingcap#7915) stats: fix histogram boundaries overflow error (pingcap#7883) ddl:support the definition of `null` change to `not null` using `alter table` (pingcap#7771) * ddl:support the definition of null change to not null using alter table ddl: add check when create table with foreign key. (pingcap#7885) * ddl: add check when create table with foreign key planner: eliminate if null on non null column (pingcap#7924) executor: fix a bug in point get (pingcap#7934) planner, executor: refine ColumnPrune for LogicalUnionAll (pingcap#7930) executor: fix panic when limit is too large (pingcap#7936) ddl: add TiDB version to metrics (pingcap#7902) stats: limit the length of sample values (pingcap#7931) vendor: update tipb (pingcap#7893) planner: support the Group and GroupExpr for the cascades planner (pingcap#7917) store/tikv: log more information when other err occurs (pingcap#7948) types: fix date time parse (pingcap#7933) ddl: just print error message when ddl job is normal to calcel, to eliminate noisy log (pingcap#7875) stats: update delta info for partition table (pingcap#7947) explaintest: add explain test for partition pruning (pingcap#7505) util: move disjoint set to util package (pingcap#7950) util: add PreAlloc4Row and Insert for Chunk and List (pingcap#7916) executor: add the slow log for commit (pingcap#7951) expression: add builtin json_keys (pingcap#7776) privilege: add USAGE in `show grants` for mysql compatibility (pingcap#7955) ddl: fix invailid ddl job panic (pingcap#7940) *: move ast.NewValueExpr to standalone parser_driver package (pingcap#7952) Make the ast package get rid of the dependency of types.Datum server: allow cors http request (pingcap#7939) *: move `Statement` and `RecordSet` from ast to sqlexec package (pingcap#7970) pr suggestion update executor/aggfuncs: split unit tests to corresponding file (pingcap#7993) store/tikv: fix typo (pingcap#7990) executor, planner: clone proj schema for different children in buildProj4Union (pingcap#7999) executor: let information_schema be the first database in ShowDatabases (pingcap#7938) stats: use local feedback for partition table (pingcap#7963) executor: add unit test for aggfuncs (pingcap#7966) server: add log for binary execute statement (pingcap#7987) admin: refine admin check decoder (pingcap#7862) executor: improve wide table insert & update performance (pingcap#7935) ddl: fix reassigned partition id in `truncate table` does not take effect (pingcap#7919) fix reassigned partition id in truncate table does not take effect add changelog for 2.1.0 rc4 (pingcap#8020) *: make parser package dependency as small as possible (pingcap#7989) parser: support `:=` in the `set` syntax (pingcap#8018) According to MySQL document, `set` use the = assignment operator, but the := assignment operator is also permitted stats: garbage collect stats for partition table (pingcap#7962) docs: add the proposal for the column pool (pingcap#7988) expression: refine built-in func truncate to support uint arg (pingcap#8000) stats: support show stats for partition table (pingcap#8023) stats: update error rate for partition table (pingcap#8022) stats: fix estimation for out of range point queries (pingcap#8015) *: move parser to a separate repository (pingcap#8036) executor: fix wrong result when index join on union scan. (pingcap#8031) Do not modify Plan of dataReaderBuilder directly, because it would impact next batch of outer rows, as well as other concurrent inner workers. Instead, build a local child builder to store the child plan. planner: fix a panic of a cached prepared statement with IndexScan (pingcap#8017) *: fix the issue of executing DDL after executing SQL failure in txn (pingcap#8044) * ddl, executor: fix the issue of executing DDL after executing SQL failure in txn add unit test remove debug info add like evaluator case sensitive test ddl, domain: make schema correct after canceling jobs (pingcap#7997) unit test fix code format proposal: maintaining histograms in plan. (pingcap#7605) support _tidb_rowid for table scan range (pingcap#8047) var rename fix

…#7952) Make the ast package get rid of the dependency of types.Datum

*: move ast.NewValueExpr to standalone parser_driver package

a27af2d

Make the ast package get rid of the dependency of types.Datum

zz-jason reviewed Oct 18, 2018

View reviewed changes

tiancaiamao added the status/all tests passed label Oct 18, 2018

zhexuany reviewed Oct 19, 2018

View reviewed changes

jackysp reviewed Oct 19, 2018

View reviewed changes

tiancaiamao added type/enhancement The issue or PR belongs to an enhancement. status/LGT1 Indicates that a PR has LGTM 1. component/parser labels Oct 19, 2018

jackysp reviewed Oct 19, 2018

View reviewed changes

types/parser_driver/value_expr.go Outdated

@@ -0,0 +1,163 @@

// Copyright 2015 PingCAP, Inc.

Copy link

Member

jackysp Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's 2018 now.

tiancaiamao added 2 commits October 19, 2018 19:23

address comment

f6c8e1c

Merge branch 'master' into parser-driver

ba6f58c

jackysp approved these changes Oct 19, 2018

View reviewed changes

tiancaiamao merged commit 25118db into pingcap:master Oct 19, 2018

tiancaiamao deleted the parser-driver branch October 19, 2018 11:37

tiancaiamao added a commit to tiancaiamao/tidb that referenced this pull request Nov 21, 2018

*: move ast.NewValueExpr to standalone parser_driver package (pingcap…

240db91

…#7952) Make the ast package get rid of the dependency of types.Datum

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: move ast.NewValueExpr to standalone parser_driver package #7952

*: move ast.NewValueExpr to standalone parser_driver package #7952

tiancaiamao commented Oct 18, 2018

tiancaiamao commented Oct 18, 2018

zz-jason Oct 18, 2018

tiancaiamao Oct 18, 2018

alivxxx Oct 19, 2018

zz-jason Oct 18, 2018

tiancaiamao Oct 18, 2018 •

edited

Loading

zz-jason Oct 18, 2018

tiancaiamao Oct 18, 2018

zz-jason Oct 18, 2018

tiancaiamao Oct 18, 2018

zhexuany left a comment

jackysp Oct 19, 2018

tiancaiamao Oct 19, 2018

jackysp Oct 19, 2018

jackysp left a comment

jackysp Oct 19, 2018

jackysp left a comment

*: move ast.NewValueExpr to standalone parser_driver package #7952

*: move ast.NewValueExpr to standalone parser_driver package #7952

Conversation

tiancaiamao commented Oct 18, 2018

What problem does this PR solve?

What is changed and how it works?

Check List

tiancaiamao commented Oct 18, 2018

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

tiancaiamao Oct 18, 2018 • 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

Choose a reason for hiding this comment

zhexuany 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

jackysp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackysp left a comment

Choose a reason for hiding this comment

tiancaiamao Oct 18, 2018 •

edited

Loading