add tableReferences in selectStatement #4916

jingshanglu · 2020-03-24T09:49:11Z

Changes proposed in this pull request:

add tableReferences in selectStatement
Keep tables and remove tables in the next subtask
only change for mysql ,Subtasks behind will be compatible with other db

codecov-io · 2020-03-24T10:30:08Z

Codecov Report

Merging #4916 into master will decrease coverage by 0.20%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##             master    #4916      +/-   ##
============================================
- Coverage     54.65%   54.45%   -0.21%     
- Complexity      436      440       +4     
============================================
  Files          1138     1140       +2     
  Lines         20403    20482      +79     
  Branches       3750     3765      +15     
============================================
+ Hits          11152    11154       +2     
- Misses         8547     8620      +73     
- Partials        704      708       +4

Impacted Files	Coverage Δ	Complexity Δ
...sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...rser/sql/segment/dml/JoinSpecificationSegment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...sql/parser/sql/segment/dml/JoinedTableSegment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...sql/parser/sql/segment/dml/TableFactorSegment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../parser/sql/segment/dml/TableReferenceSegment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../sql/parser/sql/statement/dml/SelectStatement.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...or/impl/keygen/BaseGeneratedKeyTokenGenerator.java	`66.66% <0.00%> (-16.67%)`	`1.00% <0.00%> (ø%)`
...phere/sharding/merge/dql/orderby/OrderByValue.java	`83.67% <0.00%> (-9.88%)`	`0.00% <0.00%> (ø%)`
.../sharding/route/engine/ShardingRouteDecorator.java	`23.21% <0.00%> (-3.91%)`	`0.00% <0.00%> (ø%)`
...r/binder/statement/dml/InsertStatementContext.java	`86.36% <0.00%> (-3.64%)`	`0.00% <0.00%> (ø%)`
... and 53 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 806c231...7377aa5. Read the comment docs.

coveralls · 2020-03-24T10:36:42Z

Pull Request Test Coverage Report for Build 10568

0 of 72 (0.0%) changed or added relevant lines in 6 files are covered.
53 unchanged lines in 9 files lost coverage.
Overall coverage decreased (-0.2%) to 57.973%

Changes Missing Coverage	Changed/Added Lines	%
shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/SelectStatement.java	1	0.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/JoinSpecificationSegment.java	2	0.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/TableFactorSegment.java	3	0.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/JoinedTableSegment.java	5	0.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/TableReferenceSegment.java	12	0.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java	49	0.0%

Files with Coverage Reduction	New Missed Lines	%
sharding-core/sharding-core-entry/src/main/java/org/apache/shardingsphere/core/shard/BaseShardingEngine.java	1	93.18%
sharding-core/sharding-core-rewrite/src/main/java/org/apache/shardingsphere/sharding/rewrite/engine/ShardingSQLRewriteEngine.java	1	95.65%
sharding-orchestration/sharding-orchestration-core/sharding-orchestration-core-registrycenter/src/main/java/org/apache/shardingsphere/orchestration/core/registrycenter/util/IpUtils.java	3	76.0%
shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java	3	0%
sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/TableRule.java	5	89.77%
sharding-scaling/sharding-scaling-core/src/main/java/org/apache/shardingsphere/shardingscaling/core/synctask/history/HistoryDataSyncTask.java	7	76.47%
sharding-core/sharding-core-common/src/main/java/org/apache/shardingsphere/core/rule/ShardingRule.java	8	85.21%
sharding-scaling/sharding-scaling-core/src/main/java/org/apache/shardingsphere/shardingscaling/core/execute/executor/channel/MemoryChannel.java	8	51.85%
sharding-core/sharding-core-route/src/main/java/org/apache/shardingsphere/sharding/route/engine/ShardingRouteDecorator.java	17	32.14%

Totals
Change from base Build 1155:	-0.2%
Covered Lines:	11874
Relevant Lines:	20482

💛 - Coveralls

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

tristaZero · 2020-03-25T03:51:47Z

...rc/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/TableReferenceSegment.java

+
+    private final Collection<JoinedTableSegment> joinedTables = new LinkedList<>();
+
+    private final Collection<SimpleTableSegment> tables = new LinkedList<>();


Is it likely to remove this member?

So? Why not now?

tristaZero · 2020-03-25T03:53:55Z

...nt/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/SelectStatement.java

@@ -44,6 +45,8 @@

    private ProjectionsSegment projections;

+    private final Collection<TableReferenceSegment> tableReferences = new LinkedList<>();


Do you think private final Collection<TableSegment> tables = new LinkedList<>(); can be deleted from this class?

Why do not you delete them right now?

It has been deleted.

But I can not find any relevant changes on this page.

tristaZero · 2020-03-25T03:57:08Z

...gsphere-sql-parser/shardingsphere-sql-parser-test/src/test/resources/sql/dml/select-join.xml

@@ -48,8 +48,8 @@
            <simple-table name="t_order" start-index="27" stop-index="33" />
            <simple-table name="t_order_item" start-index="40" stop-index="51" />
            <!-- FIXME should not has table segment for owner -->
-            <simple-table name="t_order" start-index="56" stop-index="62" />
-            <simple-table name="t_order_item" start-index="75" stop-index="86" />


Please add TODO comment to avoid forgetting this issue here.

this 2 lines should be deleted. other case will change like this. so,should i add TODO comment?

If you plan to delete them in the future, please add TODO comment for reminding.

? I guess you missed this comment

tristaZero · 2020-03-25T04:00:04Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

@@ -315,7 +326,12 @@ public ASTNode visitSelectClause(final SelectClauseContext ctx) {
            result.getProjections().setDistinctRow(isDistinct(ctx));
        }
        if (null != ctx.fromClause()) {
-            result.getTables().addAll(((CollectionValue<SimpleTableSegment>) visit(ctx.fromClause())).getValue());
+            CollectionValue<TableReferenceSegment> tableReferences = (CollectionValue<TableReferenceSegment>) visit(ctx.fromClause());
+            for (TableReferenceSegment t : tableReferences.getValue()) {


Please use the value named each for for loop.

tristaZero · 2020-03-25T04:00:09Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

+        for (TableReferenceSegment t : tableReferences.getValue()) {
+            result.getValue().addAll(t.getTables());
+        }
+//        result.combine((CollectionValue<SimpleTableSegment>) visit(ctx.tableReferences()));


Why did not you comment on this sentence instead of deleting it?

tristaZero · 2020-03-25T04:00:17Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

+        for (TableReferenceSegment t : tableReferences.getValue()) {
+            result.getTables().addAll(t.getTables());
+        }
+//        result.getTables().addAll(((CollectionValue<SimpleTableSegment>) visit(ctx.tableReferences())).getValue());


Why did not you comment on this sentence instead of deleting it?

tristaZero · 2020-03-25T04:01:28Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

@@ -279,7 +286,11 @@ public ASTNode visitSingleTableClause(final SingleTableClauseContext ctx) {
    public ASTNode visitMultipleTablesClause(final MultipleTablesClauseContext ctx) {
        CollectionValue<SimpleTableSegment> result = new CollectionValue<>();
        result.combine((CollectionValue<SimpleTableSegment>) visit(ctx.multipleTableNames()));
-        result.combine((CollectionValue<SimpleTableSegment>) visit(ctx.tableReferences()));
+        CollectionValue<TableReferenceSegment> tableReferences = (CollectionValue<TableReferenceSegment>) visit(ctx.tableReferences());
+        for (TableReferenceSegment t : tableReferences.getValue()) {


Please use the value named each for for loop.

tristaZero · 2020-03-25T04:01:49Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

-            for (JoinedTableContext each : ctx.joinedTable()) {
-                result.getValue().addAll(getTableSegments(result.getValue(), each));
+        if (!ctx.joinedTable().isEmpty()) {
+            for (JoinedTableContext jc : ctx.joinedTable()) {


Please use the value named each for for loop.

tristaZero · 2020-03-25T04:07:10Z

The approach you took is basically correct and a good start for this issue. However some comments and doubts need your feedback.
@jingshanglu

tristaZero

Please look at my comments.

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

tristaZero · 2020-03-25T10:02:24Z

...l/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/impl/MySQLDMLVisitor.java

+        TableFactorSegment result = new TableFactorSegment();
+        if (null != ctx.subquery()) {
+            SelectStatement subquery = (SelectStatement) visit(ctx.subquery());
+            result.setSubquery(new SubquerySegment(ctx.subquery().start.getStartIndex(), ctx.subquery().stop.getStopIndex(), subquery));


Please use SubqueryTableSegment

tristaZero · 2020-03-25T10:11:05Z

...rc/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/TableReferenceSegment.java

+
+    private final Collection<JoinedTableSegment> joinedTables = new LinkedList<>();
+
+    private final Collection<SimpleTableSegment> tables = new LinkedList<>();


So? Why not now?

tristaZero · 2020-03-25T10:13:09Z

...gsphere-sql-parser/shardingsphere-sql-parser-test/src/test/resources/sql/dml/select-join.xml

@@ -48,8 +48,8 @@
            <simple-table name="t_order" start-index="27" stop-index="33" />
            <simple-table name="t_order_item" start-index="40" stop-index="51" />
            <!-- FIXME should not has table segment for owner -->
-            <simple-table name="t_order" start-index="56" stop-index="62" />
-            <simple-table name="t_order_item" start-index="75" stop-index="86" />


If you plan to delete them in the future, please add TODO comment for reminding.

tristaZero · 2020-03-25T10:13:28Z

...t/java/org/apache/shardingsphere/sql/parser/integrate/engine/SQLParserParameterizedTest.java

@@ -133,6 +133,9 @@ private static boolean isPlaceholderWithoutParameter(final Object[] sqlTestParam

    @Test
    public void assertSupportedSQL() {
+        if ("select_alias_as_keyword".equals(sqlCaseId) && "MySQL".equals(databaseType)) {
+            System.out.println("dadf");


Delete system out.

tristaZero · 2020-03-25T10:13:51Z

...nt/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/SelectStatement.java

@@ -44,6 +45,8 @@

    private ProjectionsSegment projections;

+    private final Collection<TableReferenceSegment> tableReferences = new LinkedList<>();


Why do not you delete them right now?

...t/src/main/java/org/apache/shardingsphere/sql/parser/sql/segment/dml/TableFactorSegment.java

jingshanglu · 2020-03-25T11:20:00Z

@tristaZero I have changed base comment,please review.

tristaZero · 2020-03-26T03:30:34Z

...gsphere-sql-parser/shardingsphere-sql-parser-test/src/test/resources/sql/dml/select-join.xml

@@ -48,8 +48,8 @@
            <simple-table name="t_order" start-index="27" stop-index="33" />
            <simple-table name="t_order_item" start-index="40" stop-index="51" />
            <!-- FIXME should not has table segment for owner -->
-            <simple-table name="t_order" start-index="56" stop-index="62" />
-            <simple-table name="t_order_item" start-index="75" stop-index="86" />


? I guess you missed this comment

tristaZero · 2020-03-26T03:31:19Z

...nt/src/main/java/org/apache/shardingsphere/sql/parser/sql/statement/dml/SelectStatement.java

@@ -44,6 +45,8 @@

    private ProjectionsSegment projections;

+    private final Collection<TableReferenceSegment> tableReferences = new LinkedList<>();


But I can not find any relevant changes on this page.

lujingshang1 and others added 4 commits March 23, 2020 19:35

add JoinTableSegment

3311eb4

fix mysql visitor

1ea66f8

merge

a278b79

fix

0b41af1

tristaZero approved these changes Mar 25, 2020

View reviewed changes

tristaZero self-requested a review March 25, 2020 04:09

tristaZero requested changes Mar 25, 2020

View reviewed changes

lujingshang1 added 2 commits March 25, 2020 14:01

fix mysql visitor

a42a77f

format code

36ef256

jingshanglu requested a review from tristaZero March 25, 2020 07:10

tristaZero requested changes Mar 25, 2020

View reviewed changes

fix

7377aa5

jingshanglu requested a review from tristaZero March 25, 2020 11:05

tristaZero requested changes Mar 26, 2020

View reviewed changes

tristaZero approved these changes Mar 26, 2020

View reviewed changes

tristaZero merged commit 4d53d22 into apache:master Mar 26, 2020

tristaZero added type: refactor in: SQL parse labels Mar 26, 2020

tristaZero added this to the 5.0.0 milestone Mar 26, 2020

jingshanglu deleted the ref-4885 branch March 26, 2020 06:30

terrymanu modified the milestones: 5.0.0, 4.1.0 Mar 28, 2020

jingshanglu restored the ref-4885 branch April 1, 2020 04:18

jingshanglu deleted the ref-4885 branch April 14, 2020 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add tableReferences in selectStatement #4916

add tableReferences in selectStatement #4916

jingshanglu commented Mar 24, 2020

codecov-io commented Mar 24, 2020 •

edited

coveralls commented Mar 24, 2020 •

edited

tristaZero Mar 25, 2020

jingshanglu Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

jingshanglu Mar 25, 2020

tristaZero Mar 25, 2020

jingshanglu Mar 25, 2020

tristaZero Mar 26, 2020

tristaZero Mar 25, 2020

jingshanglu Mar 25, 2020 •

edited

tristaZero Mar 25, 2020

tristaZero Mar 26, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero commented Mar 25, 2020

tristaZero left a comment

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

tristaZero Mar 25, 2020

jingshanglu commented Mar 25, 2020

tristaZero Mar 26, 2020

tristaZero Mar 26, 2020


		private final Collection<JoinedTableSegment> joinedTables = new LinkedList<>();

		private final Collection<SimpleTableSegment> tables = new LinkedList<>();

		@@ -44,6 +45,8 @@

		private ProjectionsSegment projections;

		private final Collection<TableReferenceSegment> tableReferences = new LinkedList<>();

add tableReferences in selectStatement #4916

add tableReferences in selectStatement #4916

Conversation

jingshanglu commented Mar 24, 2020

codecov-io commented Mar 24, 2020 • edited

Codecov Report

coveralls commented Mar 24, 2020 • edited

Pull Request Test Coverage Report for Build 10568

💛 - Coveralls

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingshanglu Mar 25, 2020 • edited

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

Choose a reason for hiding this comment

tristaZero commented Mar 25, 2020

tristaZero 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingshanglu commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 24, 2020 •

edited

coveralls commented Mar 24, 2020 •

edited

jingshanglu Mar 25, 2020 •

edited