You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "kanha-gupta (via GitHub)" <gi...@apache.org> on 2023/04/07 06:29:55 UTC

[GitHub] [shardingsphere] kanha-gupta opened a new pull request, #25049: Select special functions support

kanha-gupta opened a new pull request, #25049:
URL: https://github.com/apache/shardingsphere/pull/25049

   Fixes #ISSUSE_ID.
   
   Changes proposed in this pull request:
     -
   
   ---
   
   Before committing this PR, I'm sure that I have checked the following options:
   - [ ] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
   - [ ] I have self-reviewed the commit code.
   - [ ] I have (or in comment I request) added corresponding labels for the pull request.
   - [ ] I have passed maven check locally : `./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e`.
   - [ ] I have made corresponding changes to the documentation.
   - [ ] I have added corresponding unit tests for my changes.
   


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1510673973

   @strongduanmu I've restored it. Please check :)


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1502501482

   Hi @kanha-gupta, can you merge master branch again? The ci error has been fixed yesterday.


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1501260835

   Hi @kanha-gupta, can you merge master branch?


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on code in PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#discussion_r1164982280


##########
sql-parser/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/impl/MySQLStatementSQLVisitor.java:
##########
@@ -942,7 +949,13 @@ public final ASTNode visitPositionFunction(final PositionFunctionContext ctx) {
     @Override
     public final ASTNode visitSubstringFunction(final SubstringFunctionContext ctx) {
         calculateParameterCount(Collections.singleton(ctx.expr()));

Review Comment:
   Can you push your code to this pr? `calculateParameterCount` is still here.



##########
test/it/optimizer/src/test/java/org/apache/shardingsphere/test/it/optimize/SQLNodeConverterEngineIT.java:
##########
@@ -193,6 +193,9 @@ private Set<String> getSupportedSQLCaseIDs() {
             result.add("select_order_by_for_nulls_last");
             result.add("select_char");
             result.add("select_weight_string");
+            result.add("select_substring");

Review Comment:
   In addition, I recommend supporting only one new SQL CASE at a time, which can help solve the problem.
   



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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on a diff in pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on code in PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#discussion_r1165004349


##########
sql-parser/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/mysql/visitor/statement/impl/MySQLStatementSQLVisitor.java:
##########
@@ -942,7 +949,13 @@ public final ASTNode visitPositionFunction(final PositionFunctionContext ctx) {
     @Override
     public final ASTNode visitSubstringFunction(final SubstringFunctionContext ctx) {
         calculateParameterCount(Collections.singleton(ctx.expr()));

Review Comment:
   Done



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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta closed pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta closed pull request #25049: Support for substring, DUAL , Spatial function
URL: https://github.com/apache/shardingsphere/pull/25049


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1507213141

   @strongduanmu Done.
   Thanks a lot for guidance :)
   Till now I have added support for a total of 5 SqlCase IDs


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on a diff in pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on code in PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#discussion_r1165004700


##########
test/it/optimizer/src/test/java/org/apache/shardingsphere/test/it/optimize/SQLNodeConverterEngineIT.java:
##########
@@ -193,6 +193,9 @@ private Set<String> getSupportedSQLCaseIDs() {
             result.add("select_order_by_for_nulls_last");
             result.add("select_char");
             result.add("select_weight_string");
+            result.add("select_substring");

Review Comment:
   Okay, I will :)



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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta closed pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta closed pull request #25049: Support for substring, DUAL , Spatial function
URL: https://github.com/apache/shardingsphere/pull/25049


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1510416145

   @strongduanmu Respected Sir, Thanks for the review. I have replied to them,please check.


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1506425556

   > ```
   > SQL Case ID : insert_with_batch_and_composite_expression
   > SQL         : INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))
   > SQL Params  : [1, 1, 'init', 2, 2, 'init']
   > 
   > Parameter markers count assertion error: 
   > 
   > Expected: is <6>
   >      but: was <8>
   > ```
   > 
   > Obviously, your transformation results in duplicate parameter counts, and you can choose to remove the calculateParameterCount call from the corresponding visit method.
   
   Hi @kanha-gupta, I take a look at this exception, you need to modify expected file for insert_with_batch_and_composite_expression case. Since you changed the parsing logic of SUBSTR, you need to change the expected file for all SQL cases that contain SUBSTR.
   


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on code in PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#discussion_r1166137744


##########
test/it/parser/src/main/resources/case/dml/select-special-function.xml:
##########
@@ -110,7 +110,14 @@
         <projections start-index="7" stop-index="35">
             <expression-projection text="SUBSTRING('foobarbar' from 4)" start-index="7" stop-index="35">
                 <expr>
-                    <function function-name="SUBSTRING" start-index="7" stop-index="35" text="SUBSTRING('foobarbar' from 4)" />
+                    <function function-name="SUBSTRING" start-index="7" stop-index="35" text="SUBSTRING('foobarbar' from 4)" >

Review Comment:
   You can modify the expected file just like:
   
   ```
   <function function-name="unix_timestamp" text="unix_timestamp(?)" literal-text="unix_timestamp('2019-10-19')" start-index="55" stop-index="71" literal-start-index="55" literal-stop-index="82">
                           <parameter>
                               <literal-expression value="2019-10-19" start-index="70" stop-index="81" />
                               <parameter-marker-expression parameter-index="0" start-index="70" stop-index="70" />
                           </parameter>
                       </function>
   ```



##########
test/it/parser/src/main/resources/sql/supported/dml/insert.xml:
##########
@@ -37,7 +37,7 @@
     <sql-case id="insert_set_without_generate_key_column" value="INSERT INTO t_order_item SET order_id = ?, user_id = ?, status = 'insert', creation_date='2017-08-08'" db-types="MySQL" />
     <sql-case id="insert_with_batch" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, ?), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss" />
     <sql-case id="insert_with_batch_and_irregular_parameters" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, 1, 'insert'), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss" />
-    <sql-case id="insert_with_batch_and_composite_expression" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))" db-types="H2,MySQL" />

Review Comment:
   Why remove prepared parameters?



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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on a diff in pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on code in PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#discussion_r1168082138


##########
test/it/parser/src/main/resources/sql/supported/dml/insert.xml:
##########
@@ -37,7 +37,7 @@
     <sql-case id="insert_set_without_generate_key_column" value="INSERT INTO t_order_item SET order_id = ?, user_id = ?, status = 'insert', creation_date='2017-08-08'" db-types="MySQL" />
     <sql-case id="insert_with_batch" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, ?), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss" />
     <sql-case id="insert_with_batch_and_irregular_parameters" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, 1, 'insert'), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss" />
-    <sql-case id="insert_with_batch_and_composite_expression" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))" db-types="H2,MySQL" />

Review Comment:
   Hi @kanha-gupta, could you follow my review proposal and restore the case here?
   



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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1506149613

   > @strongduanmu Hello, I request you to review this. Also, this PR shows the commits already merged into master even after Merging branch with master therefore Please suggest the solution as well. (Unknown error in InternalMySQLParserIT ) Thank you
   
   @kanha-gupta Can you run InternalMySQLParserIT on your local machine? Your changes seem to cause an exception in the CI.
   


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1506313499

   > ```
   > SQL Case ID : insert_with_batch_and_composite_expression
   > SQL         : INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))
   > SQL Params  : [1, 1, 'init', 2, 2, 'init']
   > 
   > Parameter markers count assertion error: 
   > 
   > Expected: is <6>
   >      but: was <8>
   > ```
   > 
   > Obviously, your transformation results in duplicate parameter counts, and you can choose to remove the calculateParameterCount call from the corresponding visit method.
   
   @strongduanmu I tried that right now, Did not work. Any other fix ?
   The error is occuring after I made changes for Substring function support, select_substring is working tho


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1510719265

   @strongduanmu I have restored it. Please check. are changes in Components XML also required ?


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1506152437

   ```
   SQL Case ID : insert_with_batch_and_composite_expression
   SQL         : INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))
   SQL Params  : [1, 1, 'init', 2, 2, 'init']
   
   Parameter markers count assertion error: 
   
   Expected: is <6>
        but: was <8>
   ```
   
   Obviously, your transformation results in duplicate parameter counts, and you can choose to remove the calculateParameterCount call from the corresponding visit method.
   
   


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1505930192

   @strongduanmu Hello, I request you to review this. 
   Also, this PR shows the commits already merged into master even after Merging branch with master therefore Please suggest the solution as well. 
   Thank 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.

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] kanha-gupta commented on pull request #25049: Support for substring function

Posted by "kanha-gupta (via GitHub)" <gi...@apache.org>.
kanha-gupta commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1502018464

   > Hi @kanha-gupta, can you merge master branch?
   
   Hello, I am working for multiple solutions. after that I will create a PR.
   I request you to please help on this https://github.com/apache/shardingsphere/pull/24970#issuecomment-1501060512


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

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [shardingsphere] strongduanmu commented on pull request #25049: Support for substring, DUAL , Spatial function

Posted by "strongduanmu (via GitHub)" <gi...@apache.org>.
strongduanmu commented on PR #25049:
URL: https://github.com/apache/shardingsphere/pull/25049#issuecomment-1515783036

   Hi @kanha-gupta, can you solve code 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.

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org