Use RexList to carry aggregation pre arguments for intermediate stage #13217
Conversation
xiangfu0
force-pushed
the
FunnelCompleteCount
branch
from
7c011f6 to
5287a86
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13217 +/- ##
============================================
+ Coverage 61.75% 62.15% +0.40%
+ Complexity 207 198 -9
============================================
Files 2436 2532 +96
Lines 133233 138976 +5743
Branches 20636 21531 +895
============================================
+ Hits 82274 86378 +4104
- Misses 44911 46141 +1230
- Partials 6048 6457 +409
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
xiangfu0
force-pushed
the
FunnelCompleteCount
branch
7 times, most recently
from
89fc9c0 to
d48b931
Compare
There was a problem hiding this comment.
Why do we need this? Are you trying to get boolean out of NULL? This won't be accurate because NULL is not equivalent to boolean false
There was a problem hiding this comment.
This issue is that boolean Literal is converted to 1 or 0 value over wire.
Ref: https://github.com/apache/pinot/blob/master/pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java#L70
There was a problem hiding this comment.
for null, it anyway won't work with type boolean.
There was a problem hiding this comment.
Changed to only handle the Integer conversion.
There was a problem hiding this comment.
The bug is on the RexExpression conversion side, where boolean value is not properly preserved. We should change RexExpressionUtils to keep the value type aligned with the data type. Also, if I read it correctly, TIMESTAMP is also not properly handled as it is not supported within LiteralContext yet
xiangfu0
force-pushed
the
FunnelCompleteCount
branch
3 times, most recently
from
2a0fcff to
c836128
Compare
Jackie-Jiang
added
the
backward-incompat
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/rules/PinotQueryRuleSets.java
Outdated
Show resolved
Hide resolved
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpression.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/pinot/calcite/rel/rules/PinotAggregateExchangeNodeInsertRule.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Trying to understand it better, why can't we reuse _functionOperands here for literals?
There was a problem hiding this comment.
correct, it will break the query plan, e.g. intermediate stage result reference.
So need to apply _functionPreArgs to _functionOperands during the query execution phase.
xiangfu0
force-pushed
the
FunnelCompleteCount
branch
from
c836128 to
50639e2
Compare
xiangfu0
changed the title
There was a problem hiding this comment.
Since it won't be backward compatible anyway, can we directly use this as operands? I think that is the general way to handle it, where we already support reading literal from operands
There was a problem hiding this comment.
This won't work for intermediate stage, which expects only one argument in the operand list with InputRef to the type of an intermediate object.
There was a problem hiding this comment.
Should we use same way to process raw input and intermediate input since we already have all operands?
Right you we assume we don't need function for raw input which is not always true
There was a problem hiding this comment.
The only difference is that first argument type in intermediate stage function operands might be different than raw input stage.
For count(*), no argument in rawInput stage, but you need one InputRef in the intermediate stage.
…move literal hints in AggCalls
xiangfu0
force-pushed
the
FunnelCompleteCount
branch
from
50639e2 to
9f35387
Compare
|
Close this since it's already implemented in #13282 |
3 participants
This PR re-implement #11105, which use hints to carry literals due to the calcite limitation.
Since the recent upgrade of Calcite library, we can use rexList((https://issues.apache.org/jira/browse/CALCITE-4334 and apache/calcite@ddb4200)) to carry original aggregation functions arguments for literals and intermediate stage with placeholder.