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 2021/05/19 07:32:05 UTC

[GitHub] [shardingsphere] Icesource opened a new pull request #10383: proofread the syntax definition of Create Table

Icesource opened a new pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383


   Fixes #6478.
   
   Changes proposed in this pull request:
   - Proofread the syntax definition of Create Table according to the SQL syntax definition of SQL Server official website
   


-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r635891544



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -124,15 +136,16 @@ onString
     ;
 
 memoryTablePrimaryKeyConstraintOption
-    : CLUSTERED withBucket?
+    : NONCLUSTERED

Review comment:
       @jingshanglu 
   In my viewpoint, The syntax “memoryTablePrimaryKeyConstraintOption” is used to parse memory optimized create table syntax. CLUSTERED is in syntax "diskTablePrimaryKeyConstraintOption".
   memoryTablePrimaryKeyConstraintOption in offical doc
   ```
   <column_constraint> ::=
    [ CONSTRAINT constraint_name ]
   {
     { PRIMARY KEY | UNIQUE }
         {   NONCLUSTERED
           | NONCLUSTERED HASH WITH (BUCKET_COUNT = bucket_count)
         }
     | [ FOREIGN KEY ]
           REFERENCES [ schema_name . ] referenced_table_name [ ( ref_column ) ]
     | CHECK ( logical_expression )
   }
   ```
   
   And CUSTERED is included in "diskTablePrimaryKeyConstraintOption":
   
   ```
   primaryKeyConstraint
       : (primaryKey | UNIQUE) (diskTablePrimaryKeyConstraintOption | memoryTablePrimaryKeyConstraintOption)
       ;
   diskTablePrimaryKeyConstraintOption
       : clusterOption? primaryKeyWithClause? primaryKeyOnClause?
       ;
   clusterOption
       : CLUSTERED | NONCLUSTERED
       ;
   ```




-- 
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] jingshanglu merged pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
jingshanglu merged pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383


   


-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r636630293



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @jingshanglu I try to extract the same code in ```columnConstraint``` and ```computedColumnConstraint``` into a new syntax. But if I do this, columnConstraintContext will change so I need to change SQLVistor too. This may not be a good idea. 
   ```columnConstraint``` and ```computedColumnConstraint``` have similar syntax but different scenarios. I don't find a good way to eliminate the same code. Do you have any suggestions?




-- 
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 #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#issuecomment-846560208


   @jingshanglu @Icesource fixed the issues you mentioned, please re-check them.


-- 
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] codecov-commenter edited a comment on pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#issuecomment-843869371


   # [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#10383](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (802af45) into [master](https://codecov.io/gh/apache/shardingsphere/commit/a7fcad6e9b188ab362e2fbf9fb83af3b97ff9109?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7fcad6) will **decrease** coverage by `0.44%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/shardingsphere/pull/10383/graphs/tree.svg?width=650&height=150&src=pr&token=ZvlXpWa7so&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master   #10383      +/-   ##
   ============================================
   - Coverage     68.81%   68.36%   -0.45%     
   - Complexity      695      697       +2     
   ============================================
     Files          1731     1743      +12     
     Lines         29545    29822     +277     
     Branches       5317     5355      +38     
   ============================================
   + Hits          20331    20388      +57     
   - Misses         7642     7867     +225     
   + Partials       1572     1567       -5     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | Complexity Δ | |
   |---|---|---|---|
   | [...atement/impl/PostgreSQLTCLStatementSQLVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWRpYWxlY3Qvc2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci1wb3N0Z3Jlc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zcWwvcGFyc2VyL3Bvc3RncmVzcWwvdmlzaXRvci9zdGF0ZW1lbnQvaW1wbC9Qb3N0Z3JlU1FMVENMU3RhdGVtZW50U1FMVmlzaXRvci5qYXZh) | `80.00% <0.00%> (-20.00%)` | `1.00% <0.00%> (ø%)` | |
   | [...atement/impl/PostgreSQLDALStatementSQLVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWRpYWxlY3Qvc2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci1wb3N0Z3Jlc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9zcWwvcGFyc2VyL3Bvc3RncmVzcWwvdmlzaXRvci9zdGF0ZW1lbnQvaW1wbC9Qb3N0Z3JlU1FMREFMU3RhdGVtZW50U1FMVmlzaXRvci5qYXZh) | `47.36% <0.00%> (-10.08%)` | `1.00% <0.00%> (ø%)` | |
   | [...end/text/distsql/rdl/RDLBackendHandlerFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktYmFja2VuZC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvcHJveHkvYmFja2VuZC90ZXh0L2Rpc3RzcWwvcmRsL1JETEJhY2tlbmRIYW5kbGVyRmFjdG9yeS5qYXZh) | `48.00% <0.00%> (-6.55%)` | `0.00% <0.00%> (ø%)` | |
   | [...roxy/frontend/mysql/err/MySQLErrPacketFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtcHJveHkvc2hhcmRpbmdzcGhlcmUtcHJveHktZnJvbnRlbmQvc2hhcmRpbmdzcGhlcmUtcHJveHktZnJvbnRlbmQtbXlzcWwvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3Byb3h5L2Zyb250ZW5kL215c3FsL2Vyci9NeVNRTEVyclBhY2tldEZhY3RvcnkuamF2YQ==) | `38.63% <0.00%> (-3.87%)` | `0.00% <0.00%> (ø%)` | |
   | [.../route/engine/type/ShardingRouteEngineFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZmVhdHVyZXMvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmcvc2hhcmRpbmdzcGhlcmUtc2hhcmRpbmctcm91dGUvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3NoYXJkaW5nc3BoZXJlL3NoYXJkaW5nL3JvdXRlL2VuZ2luZS90eXBlL1NoYXJkaW5nUm91dGVFbmdpbmVGYWN0b3J5LmphdmE=) | `67.74% <0.00%> (-1.23%)` | `0.00% <0.00%> (ø%)` | |
   | [...or/statement/impl/MySQLDALStatementSQLVisitor.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWRpYWxlY3Qvc2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci1teXNxbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9teXNxbC92aXNpdG9yL3N0YXRlbWVudC9pbXBsL015U1FMREFMU3RhdGVtZW50U1FMVmlzaXRvci5qYXZh) | `31.41% <0.00%> (-0.41%)` | `1.00% <0.00%> (ø%)` | |
   | [...phere/infra/binder/SQLStatementContextFactory.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtaW5mcmEvc2hhcmRpbmdzcGhlcmUtaW5mcmEtYmluZGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9zaGFyZGluZ3NwaGVyZS9pbmZyYS9iaW5kZXIvU1FMU3RhdGVtZW50Q29udGV4dEZhY3RvcnkuamF2YQ==) | `9.23% <0.00%> (-0.30%)` | `0.00% <0.00%> (ø%)` | |
   | [...rdingsphere/db/protocol/error/CommonErrorCode.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtZGItcHJvdG9jb2wvc2hhcmRpbmdzcGhlcmUtZGItcHJvdG9jb2wtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvZGIvcHJvdG9jb2wvZXJyb3IvQ29tbW9uRXJyb3JDb2RlLmphdmE=) | `100.00% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | [...sphere/sql/parser/core/visitor/SQLVisitorRule.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLWVuZ2luZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9jb3JlL3Zpc2l0b3IvU1FMVmlzaXRvclJ1bGUuamF2YQ==) | `100.00% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | [...ql/common/statement/dal/AnalyzeTableStatement.java](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2hhcmRpbmdzcGhlcmUtc3FsLXBhcnNlci9zaGFyZGluZ3NwaGVyZS1zcWwtcGFyc2VyLXN0YXRlbWVudC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2hhcmRpbmdzcGhlcmUvc3FsL3BhcnNlci9zcWwvY29tbW9uL3N0YXRlbWVudC9kYWwvQW5hbHl6ZVRhYmxlU3RhdGVtZW50LmphdmE=) | `0.00% <0.00%> (ø)` | `0.00% <0.00%> (ø%)` | |
   | ... and [48 more](https://codecov.io/gh/apache/shardingsphere/pull/10383/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a7fcad6...802af45](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r635899399



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @jingshanglu  ```fileTableClause``` is different from ```<computed_column_definition>```.  ```As``` in ```fileTableClause``` and ```computedColumnDefinition``` may appear at the same time.  So I think their parsing rules should be separated. Is that right?
   ```
   CREATE TABLE
       { database_name.schema_name.table_name | schema_name.table_name | table_name }
       [ AS FileTable ]
       ( {   <column_definition>
           | **<computed_column_definition>**
           | <column_set_definition>
           | [ <table_constraint> ] [ ,... n ]
           | [ <table_index> ] }
             [ ,...n ]
             [ PERIOD FOR SYSTEM_TIME ( system_start_time_column_name
                , system_end_time_column_name ) ]
         )
       [ ON { partition_scheme_name ( partition_column_name )
              | filegroup
              | "default" } ]
       [ TEXTIMAGE_ON { filegroup | "default" } ]
       [ FILESTREAM_ON { partition_scheme_name
              | filegroup
              | "default" } ]
       [ WITH ( <table_option> [ ,...n ] ) ]
   [ ; ]
   ```




-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r636615131



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @jingshanglu Of course, I try to extract the redundant code into a syntax ```commonColumnConstraint``` in the new commit. Do you think this change is reasonable? 
   
   Besides, I add the syntax ```constraintName``` after ```indexName``` in baseRule.g4 because [ CONSTRAINT constraint_name ] appeared many times.   




-- 
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] jingshanglu commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r635868297



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @Icesource Maybe we can refactor it，`fileTableClause` rule has `as`, you can give proposal and discuss together .

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -124,15 +136,16 @@ onString
     ;
 
 memoryTablePrimaryKeyConstraintOption
-    : CLUSTERED withBucket?
+    : NONCLUSTERED

Review comment:
       @Icesource CUSTERED is needed.
   offical doc 
   ```
   <column_constraint> ::=
   [ CONSTRAINT constraint_name ]
   {     { PRIMARY KEY | UNIQUE }
           [ CLUSTERED | NONCLUSTERED ]
           [
               WITH FILLFACTOR = fillfactor
             | WITH ( < index_option > [ , ...n ] )
           ]
           [ ON { partition_scheme_name ( partition_column_name )
               | filegroup | "default" } ]
     
     | [ FOREIGN KEY ]
           REFERENCES [ schema_name . ] referenced_table_name [ ( ref_column ) ]
           [ ON DELETE { NO ACTION | CASCADE | SET NULL | SET DEFAULT } ]
           [ ON UPDATE { NO ACTION | CASCADE | SET NULL | SET DEFAULT } ]
           [ NOT FOR REPLICATION ]
     
     | CHECK [ NOT FOR REPLICATION ] ( logical_expression )
   }
   ```




-- 
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] jingshanglu commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
jingshanglu commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r636166026



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -124,15 +136,16 @@ onString
     ;
 
 memoryTablePrimaryKeyConstraintOption
-    : CLUSTERED withBucket?
+    : NONCLUSTERED

Review comment:
       @Icesource Ok , i got it.

##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @Icesource  It is possible that the 'columnConstraint' has the same effect as the 'computedColumnConstraint' you defined.We can eliminate some of the redundant code.




-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r637547372



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @jingshanglu  I think this may not be a problem, because the SQL server official website also rewrites this similar syntax twice. According to the current writing method, it can maintain higher consistency and readability with the official website grammar definition. They are not the same and the scope of action is different. 




-- 
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 #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
tristaZero commented on pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#issuecomment-844704825


   Hi @Icesource Welcome. :)
   @jingshanglu So @Icesource is invited by you?


-- 
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] Icesource commented on a change in pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
Icesource commented on a change in pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#discussion_r636615131



##########
File path: shardingsphere-sql-parser/shardingsphere-sql-parser-dialect/shardingsphere-sql-parser-sqlserver/src/main/antlr4/imports/sqlserver/DDLStatement.g4
##########
@@ -91,6 +91,18 @@ columnConstraint
     : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | columnForeignKeyConstraint | checkConstraint)
     ;
 
+computedColumnConstraint
+    : (CONSTRAINT ignoredIdentifier)? (primaryKeyConstraint | computedColumnForeignKeyConstraint | checkConstraint)
+    ;
+
+computedColumnForeignKeyConstraint
+    : (FOREIGN KEY)? tableName (LP_ columnName RP_)? computedColumnForeignKeyOnAction*
+    ;
+
+computedColumnForeignKeyOnAction
+    : ON DELETE (NO ACTION | CASCADE) | ON UPDATE NO ACTION | NOT FOR REPLICATION
+    ;
+

Review comment:
       @jingshanglu Of course, I try to extract the redundant code into a syntax ```commonColumnConstraint``` in the new commit. Do you think this change is reasonable? 
   
   Besides, I add the syntax ```constraintName``` after ```indexName``` in baseRule.g4 because [ CONSTRAINT constraint_name ] appeared many times.   




-- 
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] codecov-commenter commented on pull request #10383: proofread the syntax definition of Create Table

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10383:
URL: https://github.com/apache/shardingsphere/pull/10383#issuecomment-843869371


   # [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#10383](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (05c701e) into [master](https://codecov.io/gh/apache/shardingsphere/commit/a7fcad6e9b188ab362e2fbf9fb83af3b97ff9109?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a7fcad6) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/shardingsphere/pull/10383/graphs/tree.svg?width=650&height=150&src=pr&token=ZvlXpWa7so&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@            Coverage Diff            @@
   ##             master   #10383   +/-   ##
   =========================================
     Coverage     68.81%   68.81%           
     Complexity      695      695           
   =========================================
     Files          1731     1731           
     Lines         29545    29545           
     Branches       5317     5317           
   =========================================
     Hits          20331    20331           
     Misses         7642     7642           
     Partials       1572     1572           
   ```
   
   
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [a7fcad6...05c701e](https://codecov.io/gh/apache/shardingsphere/pull/10383?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
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