[GitHub] kgyrtkirk commented on a change in pull request #1056: CALCITE-2858

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org


With regards,
Apache Git Services