kgyrtkirk commented on a change in pull request #1056: CALCITE-2858
URL: https://github.com/apache/calcite/pull/1056#discussion_r258865526
##########
File path: core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java
##########
@@ -391,13 +385,17 @@ RexNode toRex(RelInput relInput, Object o) {
}
if (map.containsKey("literal")) {
final Object literal = map.get("literal");
- final SqlTypeName sqlTypeName =
- Util.enumVal(SqlTypeName.class, (String) map.get("type"));
+ final RelDataType type = toType(typeFactory, map.get("type"));
Review comment:
this seems to be changing what the "type" was supposed to contain. it looks
like earlier, it was something like
`BOOLEAN` ; but now it's a more complete `toJson(node.getType())` so parsing
of a record written by an earlier writer will result in a ClassCastException at
this line because toType is guessing that it's a Map
IMHO before we should be getting into things which involve version
compatibility - the json should get some version field or something; to avoid
writing a convoluted reader
I don't know how many people are there who are actually using this - but how
about simply dropping backward compat for now?
Then all of those "o instanceof Boolean" and it's friends could go away as
well.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
