[GitHub] [calcite] julianhyde commented on a diff in pull request #3140: [CALCITE-5614] Serialize / deserialize Sarg

["julianhyde (via GitHub)" <gi...@apache.org>]

julianhyde commented on code in PR #3140:
URL: https://github.com/apache/calcite/pull/3140#discussion_r1163345760


##########
core/src/main/java/org/apache/calcite/rel/externalize/RelEnumTypes.java:
##########
@@ -78,6 +79,7 @@ private RelEnumTypes() {}
     register(enumByName, SqlTrimFunction.Flag.class);
     register(enumByName, TimeUnitRange.class);
     register(enumByName, TableModify.Operation.class);
+    register(enumByName, RexUnknownAs.class);

Review Comment:
   move a few lines up, to preserve alphabetical order



##########
core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java:
##########
@@ -341,10 +349,9 @@ public RelDataType toType(RelDataTypeFactory typeFactory, Object o) {
       final boolean nullable = get(map, "nullable");
       return typeFactory.createTypeWithNullability(type, nullable);
     } else {
-      final SqlTypeName sqlTypeName =
-          requireNonNull(Util.enumVal(SqlTypeName.class, (String) o),
-              () -> "unable to find enum value " + o
-                  + " in class " + SqlTypeName.class);
+      final SqlTypeName sqlTypeName = requireNonNull(
+          Util.enumVal(SqlTypeName.class, (String) o),
+          () -> "unable to find enum value " + o + " in class " + SqlTypeName.class);
       return typeFactory.createSqlType(sqlTypeName);

Review Comment:
   was this intentional? if not, can you restore the previous formatting



##########
core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java:
##########
@@ -460,12 +467,66 @@ public Object toJson(AggregateCall node) {
       return toJson((RelDataTypeField) value);
     } else if (value instanceof RelDistribution) {
       return toJson((RelDistribution) value);
-    } else {
+    } else if (value instanceof Sarg) {
+      return toJson((Sarg) value);
+    } else if (value instanceof RangeSet) {
+      return toJson((RangeSet) value);
+    } else if (value instanceof Range) {
+      return toJson((Range) value);
+    }
+    else {
       throw new UnsupportedOperationException("type not serializable: "
           + value + " (type " + value.getClass().getCanonicalName() + ")");
     }
   }
 
+  private Object toJson(Sarg node) {
+    final Map<String, @Nullable Object> map = jsonBuilder().map();
+    map.put("isComplementedPoints", node.isComplementedPoints());
+    map.put("pointCount", node.pointCount);
+    map.put("isAll", node.isAll());
+    map.put("isNone", node.isNone());
+    map.put("isPoints", node.isPoints());
+    map.put("rangeSet", toJson(node.rangeSet));
+    map.put("nullAs", node.nullAs);
+    return map;
+  }
+
+  private <C extends Comparable<C>> Object toJson(RangeSet<C> rangeSet) {
+    final List<Object> list = new ArrayList<>();
+    try {
+      for (Range<C> range : rangeSet.asRanges()) {
+        list.add(toJson(range));
+      }
+    } catch (Exception e){
+      throw new RuntimeException("Failed to serialize RangeSet: ", e);
+    }
+    return list;
+  }

Review Comment:
   formatting looks off for the next few lines. there should be space between methods, and space before `{`, and indentation of `rexLiteralObjectToString` is off.



##########
core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java:
##########
@@ -460,12 +467,66 @@ public Object toJson(AggregateCall node) {
       return toJson((RelDataTypeField) value);
     } else if (value instanceof RelDistribution) {
       return toJson((RelDistribution) value);
-    } else {
+    } else if (value instanceof Sarg) {
+      return toJson((Sarg) value);
+    } else if (value instanceof RangeSet) {
+      return toJson((RangeSet) value);
+    } else if (value instanceof Range) {
+      return toJson((Range) value);
+    }
+    else {
       throw new UnsupportedOperationException("type not serializable: "
           + value + " (type " + value.getClass().getCanonicalName() + ")");
     }
   }
 
+  private Object toJson(Sarg node) {
+    final Map<String, @Nullable Object> map = jsonBuilder().map();
+    map.put("isComplementedPoints", node.isComplementedPoints());
+    map.put("pointCount", node.pointCount);
+    map.put("isAll", node.isAll());

Review Comment:
   adding `isAll`, `isNone`, `isPoints` as separate fields is sub-optimal.
   
   `isAll`, `isNone` are never true at the same time. So there are 3 possibilities - all, none, other.
   
   `isPoints`, `isCompletementedPoints` and `pointCount` can be derived from other fields, so no need to add them to the JSON.



##########
core/src/main/java/org/apache/calcite/rel/externalize/RelJson.java:
##########
@@ -657,7 +721,7 @@ public RexNode toRex(RelOptCluster cluster, Object o) {
     final RexBuilder rexBuilder = cluster.getRexBuilder();
     if (o == null) {
       return null;
-    } else if (o instanceof Map) {
+    } else if (Map.class.isAssignableFrom(o.getClass())) {
       final Map<String, @Nullable Object> map = (Map) o;

Review Comment:
   I don't understand the reason for this change. It seems more verbose but seems to do the same thing (except when `o` is null).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org