HIVE-28197: Add deserializer to convert JSON plans to RelNodes #6067
Conversation
soumyakanti3578
force-pushed
the
deserializer-HIVE-28197
branch
from
a63b1f9 to
39b0ec0
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests failed
tests pending
labels
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
soumyakanti3578
force-pushed
the
deserializer-HIVE-28197
branch
from
8dad604 to
92b45f8
Compare
|
soumyakanti3578
changed the title
| String enable = pk.isEnable_cstr()? "ENABLE": "DISABLE"; | ||
| String validate = pk.isValidate_cstr()? "VALIDATE": "NOVALIDATE"; | ||
| String rely = pk.isRely_cstr()? "RELY": "NORELY"; | ||
| enableValidateRely.put(pk.getNn_name(), ImmutableList.of(enable, validate, rely)); |
| this(input.getCluster(), input.getTraitSet(), input.getInput(), | ||
| input.getBitSet("group"), input.getBitSetList("groups"), input.getAggregateCalls("aggs")); |
There was a problem hiding this comment.
Can the following work?
| this(input.getCluster(), input.getTraitSet(), input.getInput(), | |
| input.getBitSet("group"), input.getBitSetList("groups"), input.getAggregateCalls("aggs")); | |
| super(input); |
If yes then can I do the same on the other RelNodes?
| public HiveMultiJoin(RelInput input) { | ||
| this( | ||
| input.getCluster(), | ||
| input.getInputs(), | ||
| input.getExpression("condition"), | ||
| input.getRowType("rowType"), | ||
| (List<Pair<Integer, Integer>>) input.get("getJoinInputsForHiveMultiJoin"), | ||
| (List<JoinRelType>) input.get("getJoinTypesForHiveMultiJoin"), | ||
| input.getExpressionList("filters") | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do we to modify this class? Normally we shouldn't need to serialize/deserialize MultiJoin expressions cause they never appear in the final plan.
| static Stream<RelNode> stream(RelNode node) { | ||
| return Stream.concat( | ||
| Stream.of(node), | ||
| node.getInputs() | ||
| .stream() | ||
| .flatMap(HiveRelNode::stream) | ||
| ); | ||
| } |
There was a problem hiding this comment.
If we keep this we should add appropriate Javadoc. In addition, putting static methods in interfaces is not a good pattern; it is better to move it to a utility class.
Other than that the most common way to traverse RelNode tree is via visitor and shuttles so not sure if this kind of Stream based traversal is something that will be well adopted.
| * @param t | ||
| * @return | ||
| */ | ||
| private long getMaxNulls(RexCall call, HiveTableScan t) { |
There was a problem hiding this comment.
Why are we changing the selectivity estimator?
| RelPlanParser parser = new RelPlanParser(cluster, conf); | ||
| RelNode deserializedPlan = parser.parse(jsonPlan); | ||
| // Apply partition pruning to compute partition list in HiveTableScan | ||
| deserializedPlan = applyPartitionPruning(conf, deserializedPlan, cluster, planner); |
There was a problem hiding this comment.
Why do we need the partition list? Can't we deserialize the plan without it?
| // Apply partition pruning to compute partition list in HiveTableScan | ||
| deserializedPlan = applyPartitionPruning(conf, deserializedPlan, cluster, planner); | ||
| if (LOG.isDebugEnabled()) { | ||
| LOG.debug("Deserialized plan: \n{}", RelOptUtil.toString(deserializedPlan)); |
There was a problem hiding this comment.
Consider removing logging from this API. Same reasons as the one mentioned before.
| return null; | ||
| } | ||
|
|
||
| return HiveRelEnumTypes.toEnum(enumName); |
There was a problem hiding this comment.
The use of HiveRelEnumTypes seems a bit of an overkill. Can't we simply create the instance directly and drop the entire RelEnumTypes copy?
| return HiveRelEnumTypes.toEnum(enumName); | |
| return HiveTableScanTrait.valueOf(enumName); |
| if (enumName == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Are there cases where we don't serialize the trait? Can we ever have null here?
| } | ||
|
|
||
| JSONObject outJSONObject = new JSONObject(new LinkedHashMap<>()); | ||
| outJSONObject.put("CBOPlan", serializeWithPlanWriter(plan, new HiveRelJsonImpl())); |
There was a problem hiding this comment.
I don't think we need the extra wrapping attribute for "CBOPlan".
What changes were proposed in this pull request?
Added a deserializer to convert JSON plans to logical plans (RelNodes)
Why are the changes needed?
While we can serialize a plan to JSON with
explain cbo formatted, we didn't have a deserializer to convert back to a RelNode.Does this PR introduce any user-facing change?
No
How was this patch tested?