[CALCITE-5230] Fix PERCENTILE_DISC return type derivation #2868

itiels · 2022-08-09T11:37:14Z

Changed the return type of PERCENTILE_DISC to the type of the order_by_expression.

julianhyde · 2022-10-02T22:15:30Z

core/src/main/java/org/apache/calcite/sql/type/ReturnTypes.java

@@ -988,4 +990,15 @@ public static SqlCall stripSeparator(SqlCall call) {
      return relDataType;
    }
  };
+
+  public static final SqlReturnTypeInference PERCENTILE_DISC = opBinding -> {
+    if (opBinding instanceof SqlWithinGroupOperator.PercentileDiscCallBinding) {


This 'if .. instanceof ... else if ... instanceof' pattern isn't good. How about adding a method getCollationExpression() to SqlCallBinding, with a default implementation that throws. Then you can remove both getCollationType methods.

I can't implement getCollationExpression in PercentileDiscAggCallBinding, as I don't have the collation expression there. (maybe i'm missing something).
I can however add getCollationType to the base class which will remove the need to use the instance of pattern here.

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

julianhyde · 2022-10-02T22:21:28Z

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

+    } else {
+      return validateOperands(validator, scope, call);
+    }
+  }


There's a lot of duplicated code here. Maybe if you replace new SqlCallBinding(validator, scope, call) in the base method with a protected method, you can save all that duplication?

I don't think we can, because the validation/derivation is needed to be done in terms of the inner operator. even if we supply the correct SqlCallBinding to validateOperands, it will still be done in the context of the outer operator(SqlWithinGroupOperator)

libenchao

@itiels Thanks for your PR, generally it looks good now, I only left one minor comment.

Besides, PERCENTILE_COUT also should be fixed, we can do it in this issue, or a follow-up issue.

libenchao · 2022-11-01T03:51:14Z

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

@@ -2303,7 +2303,7 @@ public class SqlStdOperatorTable extends ReflectiveSqlOperatorTable {
   */
  public static final SqlAggFunction PERCENTILE_DISC =
      SqlBasicAggFunction
-          .create(SqlKind.PERCENTILE_DISC, ReturnTypes.DOUBLE,
+          .create(SqlKind.PERCENTILE_DISC, ReturnTypes.PERCENTILE_DISC,


The java doc should also be adjusted.

Thanks, fixed it

itiels · 2022-11-11T10:17:07Z

Thanks @libenchao for the review.

I've fixed the comment.
As for PERCENTILE_CONT, based on conversation with Julian in the Jira ticket, there is nothing to be done.

Is there anything else that I need/should to do for this PR?

libenchao · 2022-11-13T04:45:38Z

As for PERCENTILE_CONT, based on conversation with Julian in the Jira ticket, there is nothing to be done.

I'm not sure about this conclusion, I've checked the SQL standard (2011), 10.9:

If PERCENTILE_CONT is specified, then:

Let ROW0 be the greatest exact numeric value with scale 0 (zero) that is less than or equal
to NVE*(N–1). Let ROWLIT0 be a representing ROW0.

Let ROW1 be the least exact numeric value with scale 0 (zero) that is greater than or equal to NVE*(N–1). Let ROWLIT1 be a representing ROW1.

Let FACTOR be an representing NVE*(N–1)–ROW0.

The result is the result of the
( WITH TEMPTABLE(X, Y) AS
( SELECT ROW_NUMBER()
OVER (ORDER BY WSP) - 1, TXCOLNAME
FROM TXANAME )
SELECT CAST ( T0.Y + FACTOR * (T1.Y - T0.Y) AS DT ) FROM TEMPTABLE T0, TEMPTABLE T1
WHERE T0.ROWNUMBER = ROWLIT0
AND T1.ROWNUMBER = ROWLIT1 )

We can see that the standard is using CAST ( T0.Y + FACTOR * (T1.Y - T0.Y) AS DT ) as the result for PERCENTILE_COUT, whose type is the same as the sort key.

And also I've checked some databases, and found: oracle and postgrelsql are consistent with the standard.

There are some databases, such as server sql, are declaring the return type which is not consistent with the standard.

Hence, IMO, we should make it consistent with the standard by default. Of course if could be extensible for some dialects, if we want to achieve that, maybe the proper place to add the extension is in RelDataTypeSystem

libenchao · 2022-11-28T14:32:41Z

@itiels Sorry to ping you.
We plan to release 1.33 recently, it would be great if we can make it in this release. Do you have any comments about my last comment?

itiels · 2022-11-28T15:31:45Z

HI @libenchao,
Sorry for the late reply.

I agree with your conclusion, however I prefer to do that in a separate PR if it's ok with you.

libenchao · 2022-11-29T02:08:43Z

I'm ok with finishing it to another separate issue.
This PR now looks good to me. The CI failure seems unrelated, I'll merge it.

julianhyde · 2022-11-29T08:45:29Z

@libenchao when you merge, please change the commit message to something that doesn’t start with “fix”

libenchao · 2022-11-29T08:55:23Z

@julianhyde Very thanks for the reminder 👍 , I did plan to do that while merging.

itiels force-pushed the CALCITE-5230 branch 3 times, most recently from cfaae07 to fdf2b63 Compare August 11, 2022 15:00

julianhyde force-pushed the main branch from df83308 to 966ec17 Compare August 11, 2022 21:37

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

julianhyde reviewed Oct 2, 2022

View reviewed changes

itiels force-pushed the CALCITE-5230 branch 2 times, most recently from 5c86873 to b0e6502 Compare October 3, 2022 10:45

libenchao reviewed Nov 1, 2022

View reviewed changes

[CALCITE-5230] Fix PERCENTILE_DISC return type derivation

bc23562

itiels force-pushed the CALCITE-5230 branch from b0e6502 to bc23562 Compare November 11, 2022 10:11

libenchao self-assigned this Nov 28, 2022

libenchao approved these changes Nov 29, 2022

View reviewed changes

libenchao closed this in 9161a6f Nov 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CALCITE-5230] Fix PERCENTILE_DISC return type derivation #2868

[CALCITE-5230] Fix PERCENTILE_DISC return type derivation #2868

Uh oh!

itiels commented Aug 9, 2022

Uh oh!

julianhyde Oct 2, 2022

Uh oh!

itiels Oct 3, 2022 •

edited

Loading

Uh oh!

Uh oh!

julianhyde Oct 2, 2022

Uh oh!

itiels Oct 3, 2022

Uh oh!

libenchao left a comment

Uh oh!

libenchao Nov 1, 2022

Uh oh!

itiels Nov 11, 2022

Uh oh!

itiels commented Nov 11, 2022

Uh oh!

libenchao commented Nov 13, 2022

Uh oh!

libenchao commented Nov 28, 2022

Uh oh!

itiels commented Nov 28, 2022

Uh oh!

libenchao commented Nov 29, 2022

Uh oh!

julianhyde commented Nov 29, 2022

Uh oh!

libenchao commented Nov 29, 2022

Uh oh!

Uh oh!

[CALCITE-5230] Fix PERCENTILE_DISC return type derivation #2868

[CALCITE-5230] Fix PERCENTILE_DISC return type derivation #2868

Uh oh!

Conversation

itiels commented Aug 9, 2022

Uh oh!

julianhyde Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

itiels Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

julianhyde Oct 2, 2022

Choose a reason for hiding this comment

Uh oh!

itiels Oct 3, 2022

Choose a reason for hiding this comment

Uh oh!

libenchao left a comment

Choose a reason for hiding this comment

Uh oh!

libenchao Nov 1, 2022

Choose a reason for hiding this comment

Uh oh!

itiels Nov 11, 2022

Choose a reason for hiding this comment

Uh oh!

itiels commented Nov 11, 2022

Uh oh!

libenchao commented Nov 13, 2022

Uh oh!

libenchao commented Nov 28, 2022

Uh oh!

itiels commented Nov 28, 2022

Uh oh!

libenchao commented Nov 29, 2022

Uh oh!

julianhyde commented Nov 29, 2022

Uh oh!

libenchao commented Nov 29, 2022

Uh oh!

Uh oh!

itiels Oct 3, 2022 •

edited

Loading