You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/08/19 03:52:34 UTC

[GitHub] [calcite] amaliujia opened a new pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

amaliujia opened a new pull request #2113:
URL: https://github.com/apache/calcite/pull/2113


   https://issues.apache.org/jira/browse/CALCITE-4176


----------------------------------------------------------------
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] [calcite] amaliujia closed pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia closed pull request #2113:
URL: https://github.com/apache/calcite/pull/2113


   


----------------------------------------------------------------
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] [calcite] amaliujia commented on pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#issuecomment-689993485


   @danny0405 
   
   Got really busy recently and I can try to finish this one before next release. If there is an urgency that requires this change, I might be able to pick this one earlier. 


----------------------------------------------------------------
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] [calcite] amaliujia commented on pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#issuecomment-675836435


   R: @danny0405 
   
   Do you have any suggestions on CALCITE-4176? Espeically the idea to make the argument list of SESSION table function as: `data, timecol, key optional, size`?


----------------------------------------------------------------
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] [calcite] danny0405 commented on pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
danny0405 commented on pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#issuecomment-689968819


   Hi, @amaliujia , what is the progress of this PR ?


----------------------------------------------------------------
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] [calcite] amaliujia edited a comment on pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia edited a comment on pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#issuecomment-675836435


   R: @danny0405 
   
   Do you have any suggestions on CALCITE-4176? Especially the idea to make the argument list of SESSION table function as: `data, timecol, key optional, size`?


----------------------------------------------------------------
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] [calcite] amaliujia commented on pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia commented on pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#issuecomment-699007258


   tracking in https://github.com/apache/calcite/pull/2164


----------------------------------------------------------------
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] [calcite] amaliujia commented on a change in pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#discussion_r472664692



##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -10435,32 +10435,30 @@ private void checkCustomColumnResolving(String table) {
         + "size => interval '1' hour))")
         .fails("Param 'data' not found in function 'SESSION'; did you mean 'DATA'\\?");
     sql("select * from table(\n"
-        + "^session(\n"
+        + "session(\n"
         + "data => table orders,\n"
         + "key => descriptor(productid),\n"

Review comment:
       I found that optional named argument is not working here, even though I have do 
   
   ```
       @Override public boolean isOptional(int i) {
         return false;
         return i == 2;
       }
   ```
   I expect Calcite can detect required `timecol` has missing. I haven't figured out the reason of this.
   




----------------------------------------------------------------
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] [calcite] danny0405 commented on a change in pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#discussion_r474651986



##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -10435,32 +10435,30 @@ private void checkCustomColumnResolving(String table) {
         + "size => interval '1' hour))")
         .fails("Param 'data' not found in function 'SESSION'; did you mean 'DATA'\\?");
     sql("select * from table(\n"
-        + "^session(\n"
+        + "session(\n"
         + "data => table orders,\n"
         + "key => descriptor(productid),\n"

Review comment:
       would take a look this weekend.




----------------------------------------------------------------
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] [calcite] danny0405 commented on a change in pull request #2113: [CALCITE-4176] Key descriptor can be optional in SESSION table function (Rui Wang)

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2113:
URL: https://github.com/apache/calcite/pull/2113#discussion_r475031661



##########
File path: core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java
##########
@@ -10435,32 +10435,30 @@ private void checkCustomColumnResolving(String table) {
         + "size => interval '1' hour))")
         .fails("Param 'data' not found in function 'SESSION'; did you mean 'DATA'\\?");
     sql("select * from table(\n"
-        + "^session(\n"
+        + "session(\n"
         + "data => table orders,\n"
         + "key => descriptor(productid),\n"

Review comment:
       That is because `SqlSessionTableFunction.OperandMetadataImpl.checkOperandTypes` allows that for the descriptor. You should check the operands based on the operand name, using the new introduced `SqlOperandMetadata`. 




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