danny0405 commented on a change in pull request #2524:
URL: https://github.com/apache/calcite/pull/2524#discussion_r715978676



##########
File path: 
core/src/main/java/org/apache/calcite/sql/SqlSessionTableFunction.java
##########
@@ -79,5 +91,37 @@ public SqlSessionTableFunction() {
       return opName + "(TABLE table_name, DESCRIPTOR(timecol), "
           + "DESCRIPTOR(key) optional, datetime interval)";
     }
+
+    /**
+     * If partition-by clause and partition key descriptor both appear for a 
session window,
+     * validate they must be equals with each other.
+     *
+     * @param callBinding            The call binding
+     * @param partitionKeysInDescriptor partition keys which are defined in 
descriptor operand.
+     */
+    private void validateNoPartitionKeysConflict(SqlCallBinding callBinding,
+        List<SqlNode> partitionKeysInDescriptor) {
+      if (partitionKeysInDescriptor.isEmpty()) {
+        return;
+      }
+      SqlPartitionBy sqlPartitionBy = (SqlPartitionBy) callBinding.operand(0);
+      if (sqlPartitionBy.partitionList.isEmpty()) {
+        return;
+      }
+      List<String> partitionKeyNamesInClause = parsePartitionKeyNames(
+          sqlPartitionBy.partitionList);
+      List<String> partitionKeyNamesInDescriptor = parsePartitionKeyNames(
+          partitionKeysInDescriptor);
+      if (!partitionKeyNamesInClause.equals(partitionKeyNamesInDescriptor)) {
+        throw 
SqlUtil.newContextException(sqlPartitionBy.partitionList.getParserPosition(),
+            
RESOURCE.differentPartitionKeysForSessionWTF(partitionKeyNamesInClause.toString(),
+                partitionKeyNamesInDescriptor.toString()));
+      }

Review comment:
       The `RESOURCE` is used for parser, and we can drop the support for 
DESCRIPTOR(partition_key) in the patch IMO.

##########
File path: core/src/main/java/org/apache/calcite/sql/SqlWindowTableFunction.java
##########
@@ -244,5 +253,53 @@ void validateColumnNames(SqlValidator validator,
         }
       });
     }
+
+    /**
+     * Checks whether input table satisfy requirement of input semantics.
+     * Based on SQL standard 2016 Polymorphic Table Functions:
+     * the input with row semantics may not be partitioned.
+     * the input with set semantics may be partitioned on one or more columns.
+     *
+     * @param callBinding The call binding
+     * @return true if validation passes
+     */
+    boolean checkInputTableSemantic(SqlCallBinding callBinding) {
+      boolean isValid;

Review comment:
       Do we really need this ? How can people write a tumble or hop window 
function with partition by ?

##########
File path: 
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java
##########
@@ -753,6 +755,38 @@ public RexNode convertWindowFunction(
     return cx.getRexBuilder().makeCall(returnType, fun, exprs);
   }
 
+  public RexNode convertSessionWindowFunction(
+      SqlRexContext cx,
+      SqlSessionTableFunction fun,
+      SqlCall call) {
+    // The first operand of window function is actually a query, skip that.
+    final List<SqlNode> operands = Util.skip(call.getOperandList());
+    final List<RexNode> exprs =
+        convertOperands(cx, call, operands,
+            SqlOperandTypeChecker.Consistency.NONE);
+    RelDataType returnType =

Review comment:
       No need to support DESCRIPTOR(partition_key), it is more verbose than 
standard and hard to use.

##########
File path: core/src/main/codegen/templates/Parser.jj
##########
@@ -2351,7 +2399,16 @@ SqlNode TableFunctionCall(SqlParserPos pos) :
             funcType = 
SqlFunctionCategory.USER_DEFINED_TABLE_SPECIFIC_FUNCTION;
         }
     ]
-    call = NamedRoutineCall(funcType, ExprContext.ACCEPT_CURSOR)
+    (
+        {
+            s = span();
+        }
+        // Currenly, only session window table function requires input to be 
set semantics
+        <SESSION>
+        call = SetSemanticsWindowTVF(new 
SqlIdentifier(Collections.singletonList("SESSION"), s.pos()), funcType)
+    |

Review comment:
       Hi, @julianhyde , do you think we should have this separate branch for 
session window parsing ?

##########
File path: core/src/main/codegen/templates/Parser.jj
##########
@@ -2339,10 +2340,57 @@ void CompoundIdentifierType(List<SqlNode> list, 
List<SqlNode> extendList) :
     }
 }
 
+SqlNode SetSemanticsWindowTVF(SqlIdentifier wtfName, SqlFunctionCategory 
routineType) :
+{
+    final Span s;
+    SqlIdentifier name = null;
+    SqlNode tableRef;
+    SqlNode first;
+    SqlNodeList partitionList = SqlNodeList.EMPTY;
+    final ExprContext firstExprContext = ExprContext.ACCEPT_ALL;
+    final List<SqlNode> list = new ArrayList<SqlNode>();
+    SqlNode call;
+}
+{
+    { s = span(); }
+    <LPAREN>
+    [
+        [
+            LOOKAHEAD(2) name = SimpleIdentifier() <NAMED_ARGUMENT_ASSIGNMENT>
+        ]
+        (
+            tableRef = Default()
+        |

Review comment:
       `tableRef = Default()` this branch is not necessary.




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to