From 6ae68d4aa8f00496603e1e3caf409b531727cb97 Mon Sep 17 00:00:00 2001 From: Tanner Clary Date: Thu, 24 Aug 2023 13:31:54 -0700 Subject: [PATCH] [CALCITE-5955] BigQuery PERCENTILE functions are unparsed incorrectly --- .../sql/dialect/BigQuerySqlDialect.java | 5 +++ .../calcite/sql/fun/SqlBasicAggFunction.java | 39 +++++++++++++------ .../calcite/sql/fun/SqlLibraryOperators.java | 6 ++- .../calcite/sql/fun/SqlStdOperatorTable.java | 6 ++- .../runtime/CalciteResource.properties | 2 +- .../rel/rel2sql/RelToSqlConverterTest.java | 36 +++++++++++++++++ 6 files changed, 77 insertions(+), 17 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java index 15ae0bad0b4..dfaf5133b4c 100644 --- a/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java +++ b/core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java @@ -209,6 +209,11 @@ public BigQuerySqlDialect(SqlDialect.Context context) { } unparseItem(writer, call, leftPrec); break; + case OVER: + call.operand(0).unparse(writer, leftPrec, rightPrec); + writer.keyword("OVER"); + call.operand(1).unparse(writer, leftPrec, rightPrec); + break; default: super.unparseCall(writer, call, leftPrec, rightPrec); } diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlBasicAggFunction.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlBasicAggFunction.java index 0c1009d0986..b63b37cf29c 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlBasicAggFunction.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlBasicAggFunction.java @@ -54,6 +54,7 @@ public final class SqlBasicAggFunction extends SqlAggFunction { private final boolean allowsNullTreatment; private final boolean allowsSeparator; private final boolean percentile; + private final boolean allowsFraming; //~ Constructors ----------------------------------------------------------- @@ -66,7 +67,7 @@ private SqlBasicAggFunction(String name, @Nullable SqlIdentifier sqlIdentifier, boolean requiresOrder, boolean requiresOver, Optionality requiresGroupOrder, Optionality distinctOptionality, SqlSyntax syntax, boolean allowsNullTreatment, boolean allowsSeparator, - boolean percentile) { + boolean percentile, boolean allowsFraming) { super(name, sqlIdentifier, kind, requireNonNull(returnTypeInference, "returnTypeInference"), operandTypeInference, requireNonNull(operandTypeChecker, "operandTypeChecker"), @@ -79,6 +80,7 @@ private SqlBasicAggFunction(String name, @Nullable SqlIdentifier sqlIdentifier, this.allowsNullTreatment = allowsNullTreatment; this.allowsSeparator = allowsSeparator; this.percentile = percentile; + this.allowsFraming = allowsFraming; } /** Creates a SqlBasicAggFunction whose name is the same as its kind. */ @@ -95,7 +97,7 @@ public static SqlBasicAggFunction create(String name, SqlKind kind, return new SqlBasicAggFunction(name, null, kind, returnTypeInference, null, operandTypeChecker, null, SqlFunctionCategory.NUMERIC, false, false, Optionality.FORBIDDEN, Optionality.OPTIONAL, SqlSyntax.FUNCTION, false, - false, false); + false, false, true); } //~ Methods ---------------------------------------------------------------- @@ -147,7 +149,7 @@ public SqlAggFunction withName(String name) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Sets {@link #getDistinctOptionality()}. */ @@ -156,7 +158,7 @@ SqlBasicAggFunction withDistinct(Optionality distinctOptionality) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Sets {@link #getFunctionType()}. */ @@ -165,7 +167,7 @@ public SqlBasicAggFunction withFunctionType(SqlFunctionCategory category) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, category, requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } @Override public SqlSyntax getSyntax() { @@ -178,7 +180,7 @@ public SqlBasicAggFunction withSyntax(SqlSyntax syntax) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } @Override public boolean allowsNullTreatment() { @@ -191,7 +193,7 @@ public SqlBasicAggFunction withAllowsNullTreatment(boolean allowsNullTreatment) getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Returns whether this aggregate function allows '{@code SEPARATOR string}' @@ -206,7 +208,7 @@ public SqlBasicAggFunction withAllowsSeparator(boolean allowsSeparator) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } @Override public boolean isPercentile() { @@ -219,7 +221,20 @@ public SqlBasicAggFunction withPercentile(boolean percentile) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); + } + + @Override public boolean allowsFraming() { + return allowsFraming; + } + + /** Sets {@link #allowsFraming()}. */ + public SqlBasicAggFunction withAllowsFraming(boolean allowsFraming) { + return new SqlBasicAggFunction(getName(), getSqlIdentifier(), kind, + getReturnTypeInference(), getOperandTypeInference(), + getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), + requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Sets {@link #requiresOver()}. */ @@ -228,7 +243,7 @@ public SqlBasicAggFunction withOver(boolean over) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), over, requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Sets {@link #requiresGroupOrder()}. */ @@ -237,7 +252,7 @@ public SqlBasicAggFunction withGroupOrder(Optionality groupOrder) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), groupOrder, distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } /** Sets that value to be returned when {@link #unwrap} is applied to @@ -247,6 +262,6 @@ public SqlBasicAggFunction withStatic(SqlStaticAggFunction staticFun) { getReturnTypeInference(), getOperandTypeInference(), getOperandTypeChecker(), staticFun, getFunctionType(), requiresOrder(), requiresOver(), requiresGroupOrder(), distinctOptionality, syntax, - allowsNullTreatment, allowsSeparator, percentile); + allowsNullTreatment, allowsSeparator, percentile, allowsFraming); } } diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java index a5440a211dd..1e574903d14 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java @@ -699,7 +699,8 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding, .withFunctionType(SqlFunctionCategory.SYSTEM) .withOver(true) .withPercentile(true) - .withAllowsNullTreatment(true); + .withAllowsNullTreatment(true) + .withAllowsFraming(false); /** The {@code PERCENTILE_DISC} function, BigQuery's * equivalent to {@link SqlStdOperatorTable#PERCENTILE_DISC}, @@ -713,7 +714,8 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding, .withFunctionType(SqlFunctionCategory.SYSTEM) .withOver(true) .withPercentile(true) - .withAllowsNullTreatment(true); + .withAllowsNullTreatment(true) + .withAllowsFraming(false); /** The "DATE" function. It has the following overloads: * diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java index e6ee9734e02..40ec61d6244 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlStdOperatorTable.java @@ -2265,7 +2265,8 @@ public class SqlStdOperatorTable extends ReflectiveSqlOperatorTable { OperandTypes.UNIT_INTERVAL_NUMERIC_LITERAL) .withFunctionType(SqlFunctionCategory.SYSTEM) .withGroupOrder(Optionality.MANDATORY) - .withPercentile(true); + .withPercentile(true) + .withAllowsFraming(false); /** * {@code PERCENTILE_DISC} inverse distribution aggregate function. @@ -2280,7 +2281,8 @@ public class SqlStdOperatorTable extends ReflectiveSqlOperatorTable { OperandTypes.UNIT_INTERVAL_NUMERIC_LITERAL) .withFunctionType(SqlFunctionCategory.SYSTEM) .withGroupOrder(Optionality.MANDATORY) - .withPercentile(true); + .withPercentile(true) + .withAllowsFraming(false); /** * The LISTAGG operator. String aggregator function. diff --git a/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties b/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties index 66cd5861d20..b15d616dea1 100644 --- a/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties +++ b/core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties @@ -167,7 +167,7 @@ DuplicateWindowName=Duplicate window names not allowed EmptyWindowSpec=Empty window specification not allowed DupWindowSpec=Duplicate window specification not allowed in the same window clause QualifyExpressionMustContainWindowFunction=QUALIFY expression ''{0}'' must contain a window function -RankWithFrame=ROW/RANGE not allowed with RANK, DENSE_RANK or ROW_NUMBER functions +RankWithFrame=ROW/RANGE not allowed with RANK, DENSE_RANK, PERCENTILE_CONT/DISC or ROW_NUMBER functions FuncNeedsOrderBy=RANK or DENSE_RANK functions require ORDER BY clause in window specification PartitionNotAllowed=PARTITION BY not allowed with existing window reference OrderByOverlap=ORDER BY not allowed in both base and referenced windows diff --git a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java index 1578d0112e1..ddacd6fec29 100644 --- a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java +++ b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java @@ -1312,6 +1312,42 @@ private static String toSql(RelNode root, SqlDialect dialect, .withPostgresql().ok(expectedPostgresql); } + /** Test case for + * [CALCITE-5955] + * BigQuery PERCENTILE functions are unparsed incorrectly. */ + @Test void testPercentileContWindow() { + final String partitionQuery = "select percentile_cont(\"product_id\", 0.5)\n" + + "over(partition by \"product_id\")\n" + + "from \"foodmart\".\"product\""; + final String expectedPartition = "SELECT PERCENTILE_CONT(product_id, 0.5) " + + "OVER (PARTITION BY product_id)\n" + + "FROM foodmart.product"; + final String query = "select percentile_cont(\"product_id\", 0.5) over()\n" + + "from \"foodmart\".\"product\""; + final String expected = "SELECT PERCENTILE_CONT(product_id, 0.5) OVER ()\n" + + "FROM foodmart.product"; + sql(partitionQuery).withBigQuery().withLibrary(SqlLibrary.BIG_QUERY).ok(expectedPartition); + sql(query).withBigQuery().withLibrary(SqlLibrary.BIG_QUERY).ok(expected); + } + + /** Test case for + * [CALCITE-5955] + * BigQuery PERCENTILE functions are unparsed incorrectly. */ + @Test void testPercentileDiscWindowFrameClause() { + final String partitionQuery = "select percentile_disc(\"product_id\", 0.5)\n" + + "over(partition by \"product_id\")\n" + + "from \"foodmart\".\"product\""; + final String expectedPartition = "SELECT PERCENTILE_DISC(product_id, 0.5) " + + "OVER (PARTITION BY product_id)\n" + + "FROM foodmart.product"; + final String query = "select percentile_disc(\"product_id\", 0.5) over()\n" + + "from \"foodmart\".\"product\""; + final String expected = "SELECT PERCENTILE_DISC(product_id, 0.5) OVER ()\n" + + "FROM foodmart.product"; + sql(partitionQuery).withBigQuery().withLibrary(SqlLibrary.BIG_QUERY).ok(expectedPartition); + sql(query).withBigQuery().withLibrary(SqlLibrary.BIG_QUERY).ok(expected); + } + /** Test case for * [CALCITE-2722] * SqlImplementor createLeftCall method throws StackOverflowError. */