[GitHub] [calcite] julianhyde commented on a diff in pull request #3210: [CALCITE-5564] Add parsing and validation for PERCENTILE_CONT/PERCENTILE_DISC functions (enabled in BigQuery)

["julianhyde (via GitHub)" <gi...@apache.org>]

julianhyde commented on code in PR #3210:
URL: https://github.com/apache/calcite/pull/3210#discussion_r1198012023


##########
core/src/main/java/org/apache/calcite/sql/type/OperandTypes.java:
##########
@@ -593,6 +593,48 @@ private boolean hasFractionalPart(BigDecimal bd) {
         }
       };
 
+  public static final SqlSingleOperandTypeChecker NUMERIC_UNIT_INTERVAL_NUMERIC_LITERAL =

Review Comment:
   add javadoc. also add a blank line after the declaration.



##########
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##########
@@ -131,6 +131,7 @@ AggregateInWithinGroupIllegal=WITHIN GROUP must not contain aggregate expression
 AggregateInWithinDistinctIllegal=WITHIN DISTINCT must not contain aggregate expression
 AggregateMissingWithinGroupClause=Aggregate expression ''{0}'' must contain a WITHIN GROUP clause
 WithinGroupClauseIllegalInAggregate=Aggregate expression ''{0}'' must not contain a WITHIN GROUP clause
+percentileFunctionsArgumentLimit=Percentile functions may only have 1 or 2 arguments
 AggregateIllegalInOrderBy=Aggregate expression is illegal in ORDER BY clause of non-aggregating SELECT

Review Comment:
   s/p/P/



##########
site/_docs/reference.md:
##########
@@ -2836,6 +2836,8 @@ Dialect-specific aggregate functions.
 | b | LOGICAL_OR(condition)                          | Synonym for `SOME`
 | s | MAX_BY(value, comp)                            | Synonym for `ARG_MAX`
 | s | MIN_BY(value, comp)                            | Synonym for `ARG_MIN`
+| b | PERCENTILE_CONT(value, fraction)               | 2-argument synonym for the standard `PERCENTILE_CONT` that does not use WITHIN GROUP clause and allows for null treatment
+| b | PERCENTILE_DISC(value, fraction)               | 2-argument synonym for the standard `PERCENTILE_DISC` that does not use WITHIN GROUP clause and allows for null treatment

Review Comment:
   does it make sense to add `[ RESPECT NULLS ]` to the syntax?
   
   is it possible to concisely show the mapping, e.g. that `PERCENTILE_CONT(value, fraction)` is a synonym for `PERCENTILE_CONT(fraction) WITHIN GROUP (ORDER BY value)` (or whatever)



##########
core/src/main/codegen/templates/Parser.jj:
##########
@@ -4056,6 +4056,59 @@ SqlCall StringAggFunctionCall() :
     }
 }
 
+/**

Review Comment:
   please include some brief example syntax.



##########
testkit/src/main/java/org/apache/calcite/sql/parser/SqlParserTest.java:
##########
@@ -748,6 +748,50 @@ protected static SortedSet<String> keywords(@Nullable String dialect) {
         .fails("(?s)Encountered \"\\*\" at .*");
   }
 
+  @Test void testPercentileCont() {
+    sql("select percentile_cont(.5) within group (order by 3) from t")
+        .ok("SELECT PERCENTILE_CONT(0.5) WITHIN GROUP (ORDER BY 3)\n"
+  +

Review Comment:
   '+' should be indented



##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -6139,27 +6139,35 @@ private SqlNode navigationInDefine(SqlNode node, String alpha) {
     }
 
     if (op.isPercentile()) {

Review Comment:
   This code is now sufficiently complex that a couple of examples would be useful.



##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6540,6 +6540,64 @@ void testGroupExpressionEquivalenceParams() {
         .fails("'PERCENTILE_DISC' requires precisely one ORDER BY key");
   }
 
+  @Test void testPercentileFunctionsBigQuery() {
+    final SqlOperatorTable opTable = operatorTableFor(SqlLibrary.BIG_QUERY);

Review Comment:
   the examples over `unnest(array)` are appropriate for `big-query.iq` but are not typical of actual usage. for these tests can you switch to queries that use `emp`. Compute the percentile of sal of each employee within their department and within the whole org.



-- 
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: commits-unsubscribe@calcite.apache.org

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