[multistage] Fixing the bytes literal usage in leaf stage #11732
Conversation
xiangfu0
changed the title
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
3 times, most recently
from
d145798
to
de1fe30
Compare
Codecov Report
@@ Coverage Diff @@
## master #11732 +/- ##
=============================================
- Coverage 63.11% 14.48% -48.64%
+ Complexity 1115 201 -914
=============================================
Files 2342 2342
Lines 125899 125907 +8
Branches 19364 19367 +3
=============================================
- Hits 79464 18237 -61227
- Misses 40780 106120 +65340
+ Partials 5655 1550 -4105
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1514 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
2 times, most recently
from
d89d213
to
1f1b572
Compare
...uery-planner/src/main/java/org/apache/pinot/query/planner/serde/ProtoSerializationUtils.java
Outdated
Show resolved
Hide resolved
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
6 times, most recently
from
4424df9
to
e7290eb
Compare
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
from
e7290eb
to
4db5ef6
Compare
...y-runtime/src/main/java/org/apache/pinot/query/runtime/operator/operands/LiteralOperand.java
Outdated
Show resolved
Hide resolved
| server1.start(); | ||
| // Start server1 to ensure the next server will have a different port. |
There was a problem hiding this comment.
I saw port AlreadyBind issue from here in the tests.
...uery-planner/src/main/java/org/apache/pinot/query/planner/serde/ProtoSerializationUtils.java
Outdated
Show resolved
Hide resolved
| @@ -28,7 +29,22 @@ public class LiteralOperand implements TransformOperand { | |||
|
|
|||
| public LiteralOperand(RexExpression.Literal rexExpression) { | |||
| _resultType = rexExpression.getDataType(); | |||
| _value = rexExpression.getValue(); | |||
| _value = convert(rexExpression.getValue()); | |||
There was a problem hiding this comment.
We want to convert the value to the internal type. I think TIMESTAMP might not be handled correctly right now, where the raw value is not really the external value format Timestamp, so we may add a special case for that and add a TODO to fix that in the future
| _value = convert(rexExpression.getValue()); | |
| _value = _resultType.toInternal(rexExpression.getValue()); |
There was a problem hiding this comment.
+1. this and the rest of the places, ProtoSer/De only provide an identical mapping from ser/de perspective. for this particular situation, we need to leverage the RexExpression.Literal structure, which not only encode the value but also the type
we also need to make sure that proto ser/de only accepts particular type -->
if the data is ByteArray, byte[], or ByteString which can all ser/de into ByteString we should not allow that --> e.g. need to make sure the Ser/De is symmetrics
particularly in this case we should not allow the object to be GeogeorianCalendar --> it shouldl explicitly converted into long before setting it into the Literal
There was a problem hiding this comment.
Changed the proto serde to generate internal data type directly
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
4 times, most recently
from
8b2ddfe
to
5923197
Compare
xiangfu0
force-pushed
the
fixing-binary-literal-leaf-stage
branch
from
5923197
to
a52c914
Compare
The literal bytes passed from planner is a wrapped proto ByteString. This should be converted to Pinot internal ByteArray at inside
LiteralOperandto ensure the bytes RexExpression returns same internal type.