[CALCITE-4171] Support named parameters for table window functions #2103

danny0405 · 2020-08-10T12:44:55Z

Changes SqlArgumentAssignmentOperator to allow non-scala query as operand
In SqlCallBinding, matches the permuted operand by name with name matcher
Refactor SqlWindowTableFunction and its sub-class to reuse same logic
Add SqlOperator#paramFallbackToDefault to decide if to replace the
non-specified param with a DEFAULT call, for SqlWindowTableFunction,
it always return false

The root cause is that the COALESCE operand type was wrongly replaced by `SqlToRelConverter#adjustInputRef`, actually, for an agg as bb root, there is no need to do such adjust. Because the nullability does not change and the agg type is not same with the bb's scope. Tweak the `#adjustInputRef` to only fix type nullability, if there are cases that the type name also changes, just return the original node and let the subsequent conversion work flow throw.

amaliujia

LGTM

amaliujia · 2020-08-10T17:45:30Z

core/src/main/java/org/apache/calcite/sql/fun/SqlArgumentAssignmentOperator.java

@@ -47,4 +47,8 @@
    writer.keyword(getName());
    call.operand(0).unparse(writer, getRightPrec(), rightPrec);
  }
+
+  @Override public boolean argumentMustBeScalar(int ordinal) {
+    return false;


This is really nice. I actually tried to add named parameter support but failed on making non-scalar case (i.e. table function) work.

I wasn't aware that this can be as simple as override this parameter in SqlArgumentAssignmentOperator.java

chunweilei · 2020-08-12T02:18:02Z

core/src/main/java/org/apache/calcite/sql/SqlWindowTableFunction.java

+      return false;
+    }
+    for (int i = 1; i < descriptors + 1; i++) {
+      final SqlNode operand = callBinding.operand(i);


Why use i < descriptors + 1 rather than i <= descriptors?

Because the descriptors represent the number of the descriptors.

i <= descriptors is better since it has fewer operations.

Hi @danny0405

any suggestions on https://issues.apache.org/jira/browse/CALCITE-3780?focusedCommentId=17169620&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17169620?

Your change will assume that the structure of window table functions be row, (ROW, DESCRIPTOR, DESCRIPTOR ..., other params), in the comment we are thinking to make key descriptor of SESSION as optional, which might mean it will be moved to the last of the arguments so it becomes SESSION(data TABLE, ts DESCRIPTOR, gap interval, key DESCRIPTOR optional)

I think it is still straight-forward when it is the form (ROW, DESCRIPTOR ..., other params), in this form, we always assume that the DESCRIPTOR has the column as the time column.

Ok. I will take a look what is the best way when authoring that PR.

chunweilei · 2020-08-12T06:20:23Z

core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java

+        + "^\"data\"^ => table orders,\n"
+        + "TIMECOL => descriptor(rowtime),\n"
+        + "SIZE => interval '2' hour))")
+        .fails("Param 'data' not found in function 'TUMBLE'; did you mean 'DATA'\\?");


does the order of parameters matter?

No, but only the first data param, because for a table function, only the first param can be a query.

Should we add some tests about it? (Ignore it if it already has)

Already added in the SqlToRelConverterTest.

chunweilei · 2020-08-12T06:22:03Z

FYI, master branch is still locked~~

amaliujia · 2020-08-12T23:53:36Z

core/src/main/java/org/apache/calcite/sql/SqlWindowTableFunction.java

-        .add("window_start", timestampType)
-        .add("window_end", timestampType)
+        .add("wstart", timestampType)
+        .add("wend", timestampType)


Note that I asked one of the authors of the "one sql to rule them all" paper. The only reason that they use wstart and wend was because of the layout for a paper. I got suggestion to use window_start and window_end.

I also believe in a JIRA or an email (sorry I cannot recall which one it is) we agreed to switch to window_start and window_end.

So I will suggest we keep window_start and window_end, which can be understood easier than wstart/wend.

Well, I didn’t notice that, can you share the discussion?

I will spend some time to search for that JIRA.

ok. I might or might not discussed this in Calcite (because I cannot find a JIRA that about it), but

This is the acknowledge from Kenneth Knowles, one of the paper authors: https://docs.google.com/document/d/138uA7VTpbF84CFrd--cz3YVe0-AQ9ALnsavaSE2JeE4/edit?disco=AAAAEAjWvxg

I started to use window_start and window_end in https://issues.apache.org/jira/browse/CALCITE-3272. Julian, as another paper author, didn't against it I think.

Thanks for the share, i have reverted the rename back.

[CALCITE-4171] Support named parameters for table window functions * Changes SqlArgumentAssignmentOperator to allow non-scala query as operand * In SqlCallBinding, matches the permuted operand by name with name matcher * Refactor SqlWindowTableFunction and its sub-class to reuse same logic * Do not patch up the SqlWindowTableFunction with DEFAULTs when sql validation

amaliujia reviewed Aug 10, 2020

View reviewed changes

danny0405 force-pushed the CALCITE-4171 branch 5 times, most recently from 283b736 to 8408237 Compare August 11, 2020 08:21

danny0405 added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Aug 11, 2020

danny0405 force-pushed the CALCITE-4171 branch from 8408237 to a8366ac Compare August 11, 2020 08:46

julianhyde force-pushed the master branch from aadb605 to 37b8cdb Compare August 11, 2020 09:56

chunweilei reviewed Aug 12, 2020

View reviewed changes

danny0405 force-pushed the CALCITE-4171 branch from a8366ac to d900aca Compare August 12, 2020 07:12

amaliujia reviewed Aug 12, 2020

View reviewed changes

danny0405 force-pushed the CALCITE-4171 branch 5 times, most recently from 42f6d0d to 60af4aa Compare August 17, 2020 05:46

danny0405 force-pushed the CALCITE-4171 branch from 60af4aa to 8ab35a7 Compare August 18, 2020 02:20

danny0405 merged commit ebbb7bb into apache:master Aug 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4171] Support named parameters for table window functions #2103

[CALCITE-4171] Support named parameters for table window functions #2103

danny0405 commented Aug 10, 2020 •

edited

amaliujia left a comment

amaliujia Aug 10, 2020 •

edited

chunweilei Aug 12, 2020 •

edited

danny0405 Aug 12, 2020

chunweilei Aug 12, 2020

amaliujia Aug 13, 2020 •

edited

danny0405 Aug 13, 2020

amaliujia Aug 13, 2020

chunweilei Aug 12, 2020

danny0405 Aug 12, 2020

chunweilei Aug 12, 2020

danny0405 Aug 12, 2020

chunweilei commented Aug 12, 2020

amaliujia Aug 12, 2020 •

edited

danny0405 Aug 13, 2020

amaliujia Aug 13, 2020

amaliujia Aug 13, 2020

danny0405 Aug 13, 2020

amaliujia Aug 13, 2020 •

edited

[CALCITE-4171] Support named parameters for table window functions #2103

[CALCITE-4171] Support named parameters for table window functions #2103

Conversation

danny0405 commented Aug 10, 2020 • edited

amaliujia left a comment

Choose a reason for hiding this comment

amaliujia Aug 10, 2020 • edited

Choose a reason for hiding this comment

chunweilei Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Aug 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chunweilei commented Aug 12, 2020

amaliujia Aug 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaliujia Aug 13, 2020 • edited

Choose a reason for hiding this comment

danny0405 commented Aug 10, 2020 •

edited

amaliujia Aug 10, 2020 •

edited

chunweilei Aug 12, 2020 •

edited

amaliujia Aug 13, 2020 •

edited

amaliujia Aug 12, 2020 •

edited

amaliujia Aug 13, 2020 •

edited