You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by GitBox <gi...@apache.org> on 2020/09/10 03:28:42 UTC
[GitHub] [shardingsphere] dongzl opened a new pull request #7371: Refactor InsertStatement, add subclass for different databases.
dongzl opened a new pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371
For #7170 .
Changes proposed in this pull request:
- refactor InsertStatement, add subclass for different databases.
- fixes unit test.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero merged pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-690849099
> , which is related to you
@tristaZero Thank you. I will skip the task of refactor InsertStatement statement.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] coveralls commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-689972293
## Pull Request Test Coverage Report for [Build 14629](https://coveralls.io/builds/33351844)
* **26** of **27** **(96.3%)** changed or added relevant lines in **11** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.003%**) to **34.074%**
---
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-oracle/src/main/java/org/apache/shardingsphere/sql/parser/oracle/visitor/impl/OracleDMLVisitor.java](https://coveralls.io/builds/33351844/source?filename=shardingsphere-sql-parser%2Fshardingsphere-sql-parser-dialect%2Fshardingsphere-sql-parser-oracle%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fshardingsphere%2Fsql%2Fparser%2Foracle%2Fvisitor%2Fimpl%2FOracleDMLVisitor.java#L117) | 2 | 3 | 66.67%
<!-- | **Total:** | **26** | **27** | **96.3%** | -->
| Totals | [](https://coveralls.io/builds/33351844) |
| :-- | --: |
| Change from base [Build 14627](https://coveralls.io/builds/33351263): | 0.003% |
| Covered Lines: | 35369 |
| Relevant Lines: | 103802 |
---
##### 💛 - [Coveralls](https://coveralls.io)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-690845187
Hi @strongduanmu Please give a bit of attention to this PR, which is related to your current work. :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-690845187
Hi @strongduanmu Please give a bit of attention to this PR, which is related to your current work. :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#discussion_r486743084
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/InsertStatement.java
##########
@@ -43,7 +43,7 @@
*/
@Getter
@Setter
-public final class InsertStatement extends AbstractSQLStatement implements DMLStatement {
+public abstract class InsertStatement extends AbstractSQLStatement {
Review comment:
Thanks for your explanation. @jingshanglu Could you give it a look to check whether there will be any conflicts?
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/InsertStatement.java
##########
@@ -43,7 +43,7 @@
*/
@Getter
@Setter
-public final class InsertStatement extends AbstractSQLStatement implements DMLStatement {
+public abstract class InsertStatement extends AbstractSQLStatement {
Review comment:
Thanks for your explanation. @jingshanglu Could you give it a look to check whether there will be any conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#discussion_r486743084
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/InsertStatement.java
##########
@@ -43,7 +43,7 @@
*/
@Getter
@Setter
-public final class InsertStatement extends AbstractSQLStatement implements DMLStatement {
+public abstract class InsertStatement extends AbstractSQLStatement {
Review comment:
Thanks for your explanation. @jingshanglu Could you give it a look to check whether there will be any conflicts?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] strongduanmu commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
strongduanmu commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-690849099
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on a change in pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on a change in pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#discussion_r486191417
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/InsertStatement.java
##########
@@ -43,7 +43,7 @@
*/
@Getter
@Setter
-public final class InsertStatement extends AbstractSQLStatement implements DMLStatement {
+public abstract class InsertStatement extends AbstractSQLStatement {
Review comment:
Since some segments are just for the particular database, it is required to review and split these segments into its child classes, IMO. Like `WithSegment` is supposed to appear in `SQLServerInsertStatement`, right?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero commented on pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#issuecomment-690845187
Hi @strongduanmu Please give a bit of attention to this PR, which is related to your current work. :-)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero merged pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] tristaZero merged pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
tristaZero merged pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [shardingsphere] dongzl commented on a change in pull request #7371: Refactor InsertStatement, add subclass for different databases.
Posted by GitBox <gi...@apache.org>.
dongzl commented on a change in pull request #7371:
URL: https://github.com/apache/shardingsphere/pull/7371#discussion_r486279545
##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-statement/src/main/java/org/apache/shardingsphere/sql/parser/sql/common/statement/dml/InsertStatement.java
##########
@@ -43,7 +43,7 @@
*/
@Getter
@Setter
-public final class InsertStatement extends AbstractSQLStatement implements DMLStatement {
+public abstract class InsertStatement extends AbstractSQLStatement {
Review comment:
Hi @tristaZero , yes, we should split these segments into child classes, I want to finish this work in another PR, because in one PR, it will be modify too much files and make the code review difficultly.
and I will communicate with jingshang right now, make sure the work not conflict.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org