[hotfix] multi stage engine serde #8689
Conversation
- fix serde issue with calcite data object - adding in correct serde test
|
|
||
| public JoinNode(int stageId) { | ||
| super(stageId); | ||
| } | ||
|
|
||
| public JoinNode(int stageId, RelDataType rowType, JoinRelType joinRelType, List<JoinClause> criteria) { | ||
| public JoinNode(int stageId, FieldSpec.DataType rowType, JoinRelType joinRelType, List<JoinClause> criteria) { |
There was a problem hiding this comment.
IIUC, RelDataType can be used to represent a field's type in Calcite and also the rowType since it encapsulates the field info of all fields ? So, replacing it here with Pinot FieldSpec.DataType for Join seems intuitive since the two join key columns will be of the same type.
However, the same change further down in TableScanNode does not seem intuitive because TableScanNode technically needs a rowType as it may not necessarily work at the single column level ?
There was a problem hiding this comment.
removed this field as it is not used.
|
|
||
| public TableScanNode(int stageId) { | ||
| super(stageId); | ||
| } | ||
|
|
||
| public TableScanNode(int stageId, RelDataType rowType, String tableName, List<String> tableScanColumns) { | ||
| public TableScanNode(int stageId, FieldSpec.DataType rowType, String tableName, List<String> tableScanColumns) { |
There was a problem hiding this comment.
Are we ok with replacing rowType with a columnType ?
There was a problem hiding this comment.
good catch. actually pinot doens't support nested schema (for now) so I think we can do a list of datatypes
There was a problem hiding this comment.
2nd thought. no need to encode this here. we will add RexExpression if needed
walterddr
force-pushed
the
hotfix_multi_stage_engine_serde
branch
from
2b12b36
to
c8c6d67
Compare
Codecov Report
@@ Coverage Diff @@
## multi_stage_query_engine #8689 +/- ##
===========================================================
Coverage ? 70.47%
Complexity ? 4723
===========================================================
Files ? 1749
Lines ? 90186
Branches ? 13402
===========================================================
Hits ? 63562
Misses ? 22246
Partials ? 4378
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
* [hotfix] serde issue with RelDataType from Calcite - fix serde issue with calcite data object - adding in correct serde test * remove rowType, as it is encoded in RexExpression already Co-authored-by: Rong Rong <rongr@startree.ai>
* [hotfix] serde issue with RelDataType from Calcite - fix serde issue with calcite data object - adding in correct serde test * remove rowType, as it is encoded in RexExpression already Co-authored-by: Rong Rong <rongr@startree.ai>
* [hotfix] serde issue with RelDataType from Calcite - fix serde issue with calcite data object - adding in correct serde test * remove rowType, as it is encoded in RexExpression already Co-authored-by: Rong Rong <rongr@startree.ai>
* [hotfix] serde issue with RelDataType from Calcite - fix serde issue with calcite data object - adding in correct serde test * remove rowType, as it is encoded in RexExpression already Co-authored-by: Rong Rong <rongr@startree.ai>
SerDeUtils has 2 limitations
@ProtoProperitesfield lookup doesn't extend into superClasschange to use RexExpression from pinot that's proto serializable. to fix item 1, item 2 will be address separately