[FLINK-33179] Throw exception when serialising or deserialising ExecNode with invalid type #23488
Conversation
...le-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/ExecNodeContext.java
Outdated
Show resolved
Hide resolved
...le-planner/src/main/java/org/apache/flink/table/planner/plan/nodes/exec/ExecNodeContext.java
Outdated
Show resolved
Hide resolved
...src/test/java/org/apache/flink/table/planner/plan/nodes/exec/UnsupportedNodesInPlanTest.java
Outdated
Show resolved
Hide resolved
...va/org/apache/flink/table/planner/plan/nodes/exec/serde/ExecNodeGraphJsonSerializerTest.java
Outdated
Show resolved
Hide resolved
| public ExecNodeContext(String value) { | ||
| this.id = null; | ||
| String[] split = value.split("_"); | ||
| if ("null".equals(split[0]) || "null".equals(split[1])) { |
There was a problem hiding this comment.
I am not very familiar with this code; you are saying that the value can come through as either null_xxx or yyy_null.
Is there not a way to test for these nulls prior to calling this.
Also I wonder if you should check for the failure of Integer.valueOf(split[1]); and give a nicer message in that case.
There was a problem hiding this comment.
Please see the discussion here: #23488 (comment)
| PlanReference.fromResource( | ||
| "/jsonplan/testInvalidTypeJsonPlan.json"))) | ||
| .hasRootCauseMessage( | ||
| "Unsupported exec node type: 'null_null'."); |
There was a problem hiding this comment.
can we test xxx_null as well, so we test the || "null".equals(split[1]) part of the if
There was a problem hiding this comment.
It is not really possible to achieve this. Please see the discussion: #23488 (comment)
| throw new TableException( | ||
| String.format( | ||
| "Can not serialize ExecNode with id: %d. Missing type, this is a bug," | ||
| + " please file a ticket.", |
There was a problem hiding this comment.
I suggest rephrasing to raising a Jira and including the url.
There was a problem hiding this comment.
The current phrasing is in line with other similar places. I would also not use a proprietary system name in the code. It's not guaranteed Apache and Flink in particular continues using JIRA.
| FlinkVersion.v1_18, | ||
| Collections.singletonList(new NoAnnotationNode())))) | ||
| .hasMessageContaining( | ||
| "Can not serialize ExecNode with id: 10. Missing type, this is a bug, please file a ticket"); |
There was a problem hiding this comment.
I suggest rephrasing to raising a Jira and including the url.
…ode with invalid type
What is the purpose of the change
The change prohibits serialising
ExecNodewith a wrong type. Previously it was possible to create anull_nulltype.Moreover it gives a better exception when deserialising such a node.
Brief change log
ExecNodeContextVerifying this change
Added a test for deserialising an
ExecNodewithnull_nulltype.Added a test for serialisation in
org.apache.flink.table.planner.plan.nodes.exec.serde.ExecNodeGraphJsonSerializerTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation