yashmayya commented on code in PR #14264:
URL: https://github.com/apache/pinot/pull/14264#discussion_r1816156864
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/sql/fun/PinotOperatorTable.java:
##########
@@ -152,7 +149,17 @@ public static PinotOperatorTable instance() {
SqlStdOperatorTable.RANK,
SqlStdOperatorTable.ROW_NUMBER,
+ // WINDOW Functions (non-aggregate)
+ SqlStdOperatorTable.LAST_VALUE,
+ SqlStdOperatorTable.FIRST_VALUE,
+ // TODO: Replace these with SqlStdOperatorTable.LEAD and
SqlStdOperatorTable.LAG when the function implementations
Review Comment:
> Does this mean IGNORE NULLS are simply ignored?
Nope, using `IGNORE NULLS` with `LAG` / `LEAD` will lead to a clear error
like this during query planning - `From line 1, column 43 to line 1, column 60:
Cannot specify IGNORE NULLS or RESPECT NULLS following 'LAG'`. This is because
the custom operators we defined return `false` for `allowsNullTreatment` which
means this Calcite validation will fail -
https://github.com/apache/calcite/blob/ef1a83f659e8771c65c2541b92d2ef9cc2a05bea/core/src/main/java/org/apache/calcite/sql/SqlNullTreatmentOperator.java#L69-L74.
I'd initially gone with simply throwing a runtime exception in Pinot's
`LagValueWindowFunction` / `LeadValueWindowFunction` runtime operators, but the
alternative chosen here suggested by @gortiz (defining our own custom
`SqlAggFunction`s) is much better because we fail fast during query planning
instead of query execution and the error is also clear.
--
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]
