clintropolis commented on code in PR #14510:
URL: https://github.com/apache/druid/pull/14510#discussion_r1268789432
##########
sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/SubstringOperatorConversion.java:
##########
@@ -52,7 +53,7 @@ public class SubstringOperatorConversion implements
SqlOperatorConversion
@Override
public SqlOperator calciteOperator()
{
- return SQL_FUNCTION;
+ return SqlStdOperatorTable.SUBSTRING;
Review Comment:
is `SQL_FUNCTION` unused now?
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java:
##########
@@ -96,37 +107,38 @@ public Aggregation toDruidAggregation(
return null;
}
- RexNode separatorNode = Expressions.fromFieldAccess(
- rexBuilder.getTypeFactory(),
- rowSignature,
- project,
- aggregateCall.getArgList().get(1)
- );
- if (!separatorNode.isA(SqlKind.LITERAL)) {
- // separator must be a literal
- return null;
- }
- String separator = RexLiteral.stringValue(separatorNode);
+ final String separator;
- if (separator == null) {
- // separator must not be null
- return null;
+ if (arguments.size() > 1) {
+ separator = RexLiteral.stringValue(
+ Expressions.fromFieldAccess(
+ rexBuilder.getTypeFactory(),
+ rowSignature,
+ project,
+ aggregateCall.getArgList().get(1)
+ )
Review Comment:
shouldn't we still be checking that the separator arg is a literal?
`RexLiteral` value methods will throw an `AssertionError` if the value is not a
literal (or cast/unary minus)
##########
sql/src/test/java/org/apache/druid/sql/calcite/DecoupledPlanningCalciteQueryTest.java:
##########
@@ -337,5 +337,25 @@ public void
testFilterOnCurrentTimestampWithIntervalArithmetic()
public void testFilterOnCurrentTimestampOnView()
{
+ }
+ @Override
+ @Ignore
+ public void testExactCountDistinctWithGroupingAndOtherAggregators()
+ {
+
+ }
Review Comment:
did these previously work with the decoupled planner and no longer do?
##########
sql/src/test/resources/calcite/tests/window/wikipediaSimplePartition.sqlTest:
##########
@@ -6,29 +6,31 @@ sql: |
FLOOR(__time TO HOUR) t,
SUM(delta) delta,
SUM(SUM(delta)) OVER (PARTITION BY countryIsoCode) totalDelta,
- LAG(FLOOR(__time TO HOUR), 2) OVER (PARTITION BY countryIsoCode)
laggardTime,
- LEAD(FLOOR(__time TO HOUR), 1) OVER (PARTITION BY countryIsoCode)
leadTime,
- FIRST_VALUE(SUM(delta)) OVER (PARTITION BY countryIsoCode) AS firstDelay,
- LAST_VALUE(SUM(delta)) OVER (PARTITION BY countryIsoCode) AS lastDelay,
- NTILE(3) OVER (PARTITION BY countryIsoCode) AS delayNTile
+ LAG(FLOOR(__time TO HOUR), 2) OVER (PARTITION BY countryIsoCode ORDER BY
FLOOR(__time TO HOUR)) laggardTime,
+ LEAD(FLOOR(__time TO HOUR), 1) OVER (PARTITION BY countryIsoCode ORDER BY
FLOOR(__time TO HOUR)) leadTime,
+ FIRST_VALUE(SUM(delta)) OVER (PARTITION BY countryIsoCode ORDER BY
FLOOR(__time TO HOUR)) AS firstDelay,
+ LAST_VALUE(SUM(delta)) OVER (PARTITION BY countryIsoCode ORDER BY
FLOOR(__time TO HOUR)) AS lastDelay,
+ NTILE(3) OVER (PARTITION BY countryIsoCode ORDER BY FLOOR(__time TO HOUR))
AS delayNTile
Review Comment:
why change test query instead of adding new one?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java:
##########
@@ -556,9 +554,9 @@ public void
testJoinOnGroupByInsteadOfTimeseriesWithFloorOnTime()
cannotVectorize();
testQuery(
- "SELECT CAST(__time AS BIGINT), m1, ANY_VALUE(dim3, 100) FROM foo
WHERE (CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT), m1) IN\n"
+ "SELECT CAST(__time AS BIGINT), m1, ANY_VALUE(dim3, 100) FROM foo
WHERE (CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT) + 1, m1) IN\n"
+ " (\n"
- + " SELECT CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT) + 0 AS t1,
MIN(m1) AS t2 FROM foo WHERE dim3 = 'b'\n"
+ + " SELECT CAST(TIME_FLOOR(__time, 'PT1H') AS BIGINT) + 1 AS t1,
MIN(m1) AS t2 FROM foo WHERE dim3 = 'b'\n"
Review Comment:
why change test query instead of adding a new test?
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/SumSqlAggregator.java:
##########
@@ -40,23 +31,20 @@
import org.apache.druid.sql.calcite.aggregation.Aggregation;
import org.apache.druid.sql.calcite.planner.Calcites;
+import javax.annotation.Nullable;
+
public class SumSqlAggregator extends SimpleSqlAggregator
{
- /**
- * We are using a custom SUM function instead of {@link
org.apache.calcite.sql.fun.SqlStdOperatorTable#SUM} to
- * work around the issue described in
https://issues.apache.org/jira/browse/CALCITE-4609. Once we upgrade Calcite
- * to 1.27.0+ we can return to using the built-in SUM function, and {@link
DruidSumAggFunction and
- * {@link DruidSumSplitter} can be removed.
- */
- private static final SqlAggFunction DRUID_SUM = new DruidSumAggFunction();
+ private static final SqlAggFunction FUNCTION = new SqlSumAggFunction(null);
Review Comment:
oh yeah, cool :+1:
##########
sql/src/test/resources/calcite/tests/window/wikipediaCumulativeOrdered.sqlTest:
##########
@@ -17,9 +17,12 @@ sql: |
CUME_DIST() OVER (PARTITION BY countryIsoCode ORDER BY SUM(delta)) AS
delayCumeDist
FROM wikipedia
GROUP BY 1, 2
- ORDER BY 1, 3
Review Comment:
why change the test query?
##########
sql/src/main/java/org/apache/druid/sql/calcite/rule/GroupByRules.java:
##########
@@ -64,6 +65,11 @@ public static Aggregation translateAggregateCall(
final boolean finalizeAggregations
)
{
+ if (!call.getCollation().getFieldCollations().isEmpty()) {
+ // TODO(gianm): validate no WITHIN GROUP elsewhere, mention location of
validation code here
Review Comment:
does this still need done?
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java:
##########
@@ -96,37 +107,38 @@ public Aggregation toDruidAggregation(
return null;
}
- RexNode separatorNode = Expressions.fromFieldAccess(
- rexBuilder.getTypeFactory(),
- rowSignature,
- project,
- aggregateCall.getArgList().get(1)
- );
- if (!separatorNode.isA(SqlKind.LITERAL)) {
- // separator must be a literal
- return null;
- }
- String separator = RexLiteral.stringValue(separatorNode);
+ final String separator;
- if (separator == null) {
- // separator must not be null
- return null;
+ if (arguments.size() > 1) {
+ separator = RexLiteral.stringValue(
+ Expressions.fromFieldAccess(
+ rexBuilder.getTypeFactory(),
+ rowSignature,
+ project,
+ aggregateCall.getArgList().get(1)
+ )
+ );
+ } else {
+ separator = "";
}
- Integer maxSizeBytes = null;
+ final HumanReadableBytes maxSizeBytes;
+
if (arguments.size() > 2) {
- RexNode maxBytes = Expressions.fromFieldAccess(
- rexBuilder.getTypeFactory(),
- rowSignature,
- project,
- aggregateCall.getArgList().get(2)
+ maxSizeBytes = HumanReadableBytes.valueOf(
+ RexLiteral.intValue(
+ Expressions.fromFieldAccess(
+ rexBuilder.getTypeFactory(),
+ rowSignature,
+ project,
+ aggregateCall.getArgList().get(2)
+ )
+ )
Review Comment:
same comment about checking for literal argument
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteArraysQueryTest.java:
##########
@@ -3159,34 +3273,26 @@ public void testUnnestWithFiltersInsideAndOutside()
skipVectorize();
testQuery(
"SELECT d3 FROM\n"
- + " (select * from druid.numfoo where dim2='a') t,\n"
+ + " (select * from druid.numfoo where dim2='a'),\n"
+ " UNNEST(MV_TO_ARRAY(dim3)) as unnested (d3)\n"
- + "WHERE t.dim1 <> 'foo'\n"
- + "AND unnested.d3 <> 'b'",
+ + "WHERE dim1 <> 'foo'\n"
+ + "AND (unnested.d3 IN ('a', 'c') OR unnested.d3 LIKE '_')",
Review Comment:
what is the reason for changing this test instead of adding a new one?
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/StringSqlAggregator.java:
##########
@@ -139,7 +151,11 @@ public Aggregation toDruidAggregation(
fieldName =
virtualColumnRegistry.getOrCreateVirtualColumnForExpression(arg, elementType);
}
- final String finalizer = StringUtils.format("if(array_length(o) == 0,
null, array_to_string(o, '%s'))", separator);
+ final String finalizer = StringUtils.format(
+ "if(array_length(o) == 0, null, array_to_string(o, %s))",
+ DruidExpression.ofStringLiteral(separator).getExpression()
Review Comment:
why wrap the string in `DruidExpression` just to get the string back out
instead of just leaving the single quotes in the format?
##########
sql/src/test/java/org/apache/druid/sql/calcite/CalciteQueryTest.java:
##########
@@ -2408,7 +2408,8 @@ public void testExactCountDistinctWithFilter()
new CountAggregatorFactory("_a0"),
and(
not(selector("d0", null, null)),
- selector("a1", "0", null)
+ selector("a1", "0", null),
+ expressionFilter("\"d1\"")
Review Comment:
hmm, what is the story on this expression filter?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
