Allow adding and subtracting timestamp types in the multi-stage engine #14782
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14782 +/- ##
============================================
+ Coverage 61.75% 63.85% +2.09%
- Complexity 207 1607 +1400
============================================
Files 2436 2704 +268
Lines 133233 150930 +17697
Branches 20636 23318 +2682
============================================
+ Hits 82274 96370 +14096
- Misses 44911 47344 +2433
- Partials 6048 7216 +1168
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // operations, so we need to define our own operators. Note that Postgres supports - on TIMESTAMP types, but not +. | ||
| // Calcite only supports such operations if the second operand is an interval (similar to Postgres for the + | ||
| // operator). | ||
| public static final SqlBinaryOperator PINOT_PLUS = |
There was a problem hiding this comment.
Do we need to explicitly register them? Or just let the scalar function register it which has very loose check?
There was a problem hiding this comment.
That works for ADD / SUB, but not for + / - since those are unique operators. We get an error like this during query compilation (sample query - SELECT ts - CAST(1234567890 AS TIMESTAMP) from airlineStats LIMIT 1;) if we try to register + / - as Pinot scalar functions -
Invalid number of arguments to function '-'. Was expecting 2 arguments
There was a problem hiding this comment.
I think that should be fixable by adding a new constructor to PinotSqlFunction and allowing caller to pass in SqlKind though.
Edit: We'd also need to define the right precedence and associativity, so maybe this way is simpler?
Edit 2: We should also be defining them as SqlMonotonicBinaryOperator, and not SqlFunction (like scalar function registry does).
ADDandSUBscalar functions in Pinot were not aliased to the Calcite standard operators+/-. This meant that the scalar function definition was used and that allowed for things likeSUB(ts_col1, ts_col2)orADD(ts_col1, ts_literal).ts_col1 - ts_col2orts_col1 + ts_col2were still not allowed since the Calcite standard operators only support these operand types - numeric and numeric, datetime types and intervals, or intervals and intervals (see here). This led to an inconsistent experience with Pinot's operators. Also, note that Postgres supports directly subtracting timestamp types (but only adding timestamps to intervals).ADDandSUB. Also, since Pinot uses long types for timestamps internally and we support such operations in the single-stage engine, this patch also adds support for timestamp types with+and-in the multi-stage engine.