add parsing for AT TIME ZONE #9477
Conversation
agavra
force-pushed
the
at_time_zone
branch
from
0cd7d0f to
46e8545
Compare
agavra
changed the title
This commit adds the custom Calcite extension for parsing AT TIME ZONE expressions, but does not yet support executing these queries.
agavra
force-pushed
the
at_time_zone
branch
from
46e8545 to
f52e7ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #9477 +/- ##
=============================================
+ Coverage 34.73% 67.14% +32.40%
- Complexity 194 4944 +4750
=============================================
Files 1910 1419 -491
Lines 101850 74027 -27823
Branches 15452 11815 -3637
=============================================
+ Hits 35379 49704 +14325
+ Misses 63483 20728 -42755
- Partials 2988 3595 +607
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| import static java.util.Objects.requireNonNull; | ||
|
|
||
|
|
||
| public class SqlAtTimeZone extends SqlSpecialOperator { |
There was a problem hiding this comment.
lets model it similar to SqlTimestampAddFunction extends SqlFunction?
any specific reason we want to model it as special operator?
There was a problem hiding this comment.
This has special syntax (it's not the usual FUN_NAME(params...)) which makes it a bit more complicated - I modeled after SqlLikeOperator which is more similar to this w.r.t syntax. I have to implement the reduceExpr to clearly indicate that the left argument comes before the operation and the right argument comes afterwards.
| { | ||
| checkNonQueryExpression(exprContext); | ||
| s.clear().add(this); | ||
| } |
There was a problem hiding this comment.
why do we need to check this. can't it be something like
SELECT * FROM tbl
WHERE (SELECT MAX(timestampWithoutTimeZone) FROM anotherTbl) AT TIME ZONE 'Pacific' BETWEEN xxx AND xxx
There was a problem hiding this comment.
I modeled this after the LIKE syntax which has that check - I'm not 100% sure why they added that restriction, because I checked Postgres and SELECT (SELECT str FROM foo) LIKE 'foo' FROM foo; works and so does nested AT TIME ZONE like your example query.
I tried these in Pinot, though, and it looks like there are other limitations that prevent us from doing that...
SELECT (SELECT str FROM foo) LIKE 'foo' FROM foo; returns the following exception in Pinot:
java.lang.ClassCastException: class org.apache.calcite.sql.SqlSelect cannot be cast to class org.apache.calcite.sql.SqlBasicCall (org.apache.calcite.sql.SqlSelect and org.apache.calcite.sql.SqlBasicCall are in unnamed module of loader 'app')
at org.apache.pinot.sql.parsers.CalciteSqlParser.toExpression(CalciteSqlParser.java:748)
at org.apache.pinot.sql.parsers.CalciteSqlParser.compileFunctionExpression(CalciteSqlParser.java:806)
at org.apache.pinot.sql.parsers.CalciteSqlParser.toExpression(CalciteSqlParser.java:748)
If you feel strongly, I can remove the check, but it won't work either way without additional work (which I think we can address in a follow-up if desired)
There was a problem hiding this comment.
for now it should be fine. since this should probably go back to calcite we can adjust later
| { | ||
| checkNonQueryExpression(exprContext); | ||
| s.clear().add(this); | ||
| } |
There was a problem hiding this comment.
for now it should be fine. since this should probably go back to calcite we can adjust later
pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java
Show resolved
Hide resolved
* add parsing for AT TIME ZONE This commit adds the custom Calcite extension for parsing AT TIME ZONE expressions, but does not yet support executing these queries.
This commit adds the custom Calcite extension for parsing
AT TIME ZONEexpressions, but does not yet support executing these queries.I implemented the
@ScalarFunctionlocally to make sure that it can work when fully wired in but I wanted to commit that separately because we should take care to make sure that we do it in a backwards compatible way. The complications here are thatAT TIME ZONEbehaves different based on whether the input type already has a time zone associated with it or not. Additionally,AT TIME ZONEreturns a timestamp with time zone for non-zoned timestamps, which means we need to support both types before getting the "SQL Standard" behavior here.Also See: postgres specficiation