gargvishesh commented on code in PR #15095:
URL: https://github.com/apache/druid/pull/15095#discussion_r1354747658
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -313,12 +328,48 @@ public RelDataType inferReturnType(SqlOperatorBinding
sqlOperatorBinding)
}
}
+ private static class TimeColIdentifer extends SqlIdentifier
+ {
+
+ public TimeColIdentifer()
+ {
+ super("__time", SqlParserPos.ZERO);
+ }
+
+ @Override
+ public <R> R accept(SqlVisitor<R> visitor)
+ {
+
+ try {
+ return super.accept(visitor);
+ }
+ catch (CalciteContextException e) {
+ if (e.getCause() instanceof SqlValidatorException) {
+ throw DruidException.forPersona(DruidException.Persona.ADMIN)
Review Comment:
Chose `ADMIN` Persona here since the exceptions in existing tests such as
`testTimeColumnAggregationsOnLookups` had had the same -- not sure why though.
If that's incorrect, shall update the tests as well.
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -339,6 +390,43 @@ private static class EarliestLatestSqlAggFunction extends
SqlAggFunction
false,
Optionality.FORBIDDEN
);
+ this.replacementAggFunc = replacementAggFunc;
+ }
+
+ @Override
+ public SqlNode rewriteCall(
+ SqlValidator validator,
+ SqlCall call
+ )
+ {
+ // Rewrite EARLIEST/LATEST to EARLIEST_BY/LATEST_BY to make
+ // reference to __time column explicit so that Calcite tracks it
+
+ if (replacementAggFunc == null) {
+ return call;
+ }
+
+ List<SqlNode> operands = call.getOperandList();
+
+ SqlParserPos pos = call.getParserPosition();
+
+ if (operands.isEmpty() || operands.size() > 2) {
+ throw InvalidSqlInput.exception(
+ "Function [%s] expects 1 or 2 arguments but found [%s]",
+ getName(),
+ operands.size()
+ );
+ }
+
+ List<SqlNode> newOperands = new ArrayList<>();
+ newOperands.add(operands.get(0));
+ newOperands.add(new TimeColIdentifer());
+
+ if (operands.size() == 2) {
+ newOperands.add(operands.get(1));
+ }
Review Comment:
Yes, the LATEST operator's signature is `LATEST(expr, maxBytes)`
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -313,12 +328,48 @@ public RelDataType inferReturnType(SqlOperatorBinding
sqlOperatorBinding)
}
}
+ private static class TimeColIdentifer extends SqlIdentifier
+ {
+
+ public TimeColIdentifer()
+ {
+ super("__time", SqlParserPos.ZERO);
+ }
+
+ @Override
+ public <R> R accept(SqlVisitor<R> visitor)
+ {
+
+ try {
+ return super.accept(visitor);
+ }
+ catch (CalciteContextException e) {
+ if (e.getCause() instanceof SqlValidatorException) {
+ throw DruidException.forPersona(DruidException.Persona.ADMIN)
+
.ofCategory(DruidException.Category.INVALID_INPUT)
+ .build(
+ e,
+ "Query could not be planned. A possible
reason is [%s]",
Review Comment:
Didn't add that since they are already there, such as
[testTimeColumnAggregationsOnLookups](https://github.com/gargvishesh/druid/blob/6e23061d48e98ec6fe53fb00a2dfd7c9dddf08c9/sql/src/test/java/org/apache/druid/sql/calcite/CalciteJoinQueryTest.java#L1491)
and
[testTimeColumnAggregationFromExtern](https://github.com/gargvishesh/druid/blob/cb050282a03fecf5d5af5678cfac6f7a73af7a43/extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/exec/MSQSelectTest.java#L1772)
##########
sql/src/main/java/org/apache/druid/sql/calcite/aggregation/builtin/EarliestLatestAnySqlAggregator.java:
##########
@@ -313,12 +328,48 @@ public RelDataType inferReturnType(SqlOperatorBinding
sqlOperatorBinding)
}
}
+ private static class TimeColIdentifer extends SqlIdentifier
+ {
+
+ public TimeColIdentifer()
+ {
+ super("__time", SqlParserPos.ZERO);
+ }
+
+ @Override
+ public <R> R accept(SqlVisitor<R> visitor)
Review Comment:
Before `validate` of an operator is invoked, the identifiers are changed
into fully qualified names with table prefixes. It's at that time the `__time`
col's existence is examined (and `accept` gets called) where a col missing
exception can potentially be triggered.
--
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]
