Copilot commented on code in PR #7532:
URL: https://github.com/apache/ignite-3/pull/7532#discussion_r2787949533
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -814,9 +810,21 @@ RexNode toRex(RelInput relInput, Object o) {
upperBound = null;
physical = false;
}
+
+ final RexWindowExclusion exclude;
+ if (window.get("exclude") != null) {
+ exclude = toRexWindowExclusion((Map)
window.get("exclude"));
+ } else {
+ exclude = RexWindowExclusion.EXCLUDE_NO_OTHER;
+ }
+
boolean distinct = (Boolean) map.get("distinct");
return rexBuilder.makeOver(type, operator, rexOperands,
partitionKeys,
- ImmutableList.copyOf(orderKeys), lowerBound,
upperBound, physical,
+ ImmutableList.copyOf(orderKeys),
+ requireNonNull(lowerBound, "lowerBound"),
+ requireNonNull(upperBound, "upperBound"),
+ requireNonNull(exclude, "exclude"),
+ physical,
Review Comment:
`toRex` now calls `requireNonNull(lowerBound/upperBound)` when deserializing
a windowed aggregate. However `toJson(RexWindow)` intentionally omits bounds
when there is no ROWS/RANGE clause, which leaves both bounds null and will
cause deserialization to fail for such windows. Consider either (a) serializing
explicit default bounds/exclusion in `toJson(RexWindow)`, or (b) in the "No
ROWS or RANGE clause" branch, set Calcite-equivalent default bounds (instead of
null) before calling `makeOver`.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -923,6 +955,116 @@ RexNode toRex(RelInput relInput, Object o) {
}
}
+ private static <C extends Comparable<C>> Sarg<C> sargFromJson(Map<String,
Object> map, RelDataType type) {
+ final String nullAs = requireNonNull((String) map.get("nullAs"),
"nullAs");
+ final List<List<String>> rangeSet =
+ requireNonNull((List<List<String>>) map.get("rangeSet"),
"rangeSet");
+ return Sarg.of(ENUM_BY_NAME.get(nullAs),
+ RelJson.<C>rangeSetFromJson(rangeSet, type));
+ }
+
+ /** Converts a JSON list to a {@link RangeSet} with supplied value typing.
*/
+ private static <C extends Comparable<C>> RangeSet<C> rangeSetFromJson(
+ List<List<String>> rangeSetsJson, RelDataType type) {
+ final ImmutableRangeSet.Builder<C> builder =
ImmutableRangeSet.builder();
+ try {
+ rangeSetsJson.forEach(list -> builder.add(rangeFromJson(list,
type)));
+ } catch (Exception e) {
+ throw new RuntimeException("Error creating RangeSet from JSON: ",
e);
+ }
+ return builder.build();
+ }
+
+ /**
+ * Creates a {@link Range} from a JSON object.
+ *
+ * <p>The JSON object is as serialized using {@link #toJson(Range)},
+ * e.g. {@code ["[", ")", 10, "-"]}.
+ */
+ private static <C extends Comparable<C>> Range<C> rangeFromJson(
+ List<String> list, RelDataType type) {
+ switch (list.get(0)) {
+ case "all":
+ return Range.all();
+ case "atLeast":
+ return Range.atLeast(rangeEndPointFromJson(list.get(1), type));
+ case "atMost":
+ return Range.atMost(rangeEndPointFromJson(list.get(1), type));
+ case "greaterThan":
+ return Range.greaterThan(rangeEndPointFromJson(list.get(1),
type));
+ case "lessThan":
+ return Range.lessThan(rangeEndPointFromJson(list.get(1),
type));
+ case "singleton":
+ return Range.singleton(rangeEndPointFromJson(list.get(1),
type));
+ case "closed":
+ return Range.closed(rangeEndPointFromJson(list.get(1), type),
+ rangeEndPointFromJson(list.get(2), type));
+ case "closedOpen":
+ return Range.closedOpen(rangeEndPointFromJson(list.get(1)),
+ rangeEndPointFromJson(list.get(2), type));
+ case "openClosed":
+ return Range.openClosed(rangeEndPointFromJson(list.get(1)),
+ rangeEndPointFromJson(list.get(2), type));
+ case "open":
+ return Range.open(rangeEndPointFromJson(list.get(1), type),
+ rangeEndPointFromJson(list.get(2), type));
+ default:
+ throw new AssertionError("unknown range type " + list.get(0));
+ }
+ }
+
+ @SuppressWarnings({"rawtypes", "unchecked"})
+ @Deprecated
+ private static <C extends Comparable<C>> C rangeEndPointFromJson(Object o)
{
+ Exception e = null;
+ for (Class clsType : VALUE_CLASSES) {
+ try {
+ return (C) OBJECT_MAPPER.readValue((String) o, clsType);
+ } catch (JsonProcessingException ex) {
+ e = ex;
+ }
+ }
+ throw new RuntimeException(
+ "Error deserializing range endpoint (did not find compatible
type): ",
+ e);
+ }
+
+ private static <C extends Comparable<C>> C rangeEndPointFromJson(Object o,
RelDataType type) {
+ Exception e;
+ try {
+ Class clsType = determineRangeEndpointValueClass(type);
+ return (C) OBJECT_MAPPER.readValue((String) o, clsType);
+ } catch (JsonProcessingException ex) {
+ e = ex;
+ }
+ throw new RuntimeException(
+ "Error deserializing range endpoint (did not find compatible
type): ",
+ e);
+ }
+
+ private static Class determineRangeEndpointValueClass(RelDataType type) {
+ SqlTypeName typeName = RexLiteral.strictTypeName(type);
+ switch (typeName) {
+ case DECIMAL:
+ return BigDecimal.class;
+ case DOUBLE:
+ return Double.class;
+ case CHAR:
+ return NlsString.class;
+ case BOOLEAN:
+ return Boolean.class;
+ case TIMESTAMP:
+ return TimestampString.class;
+ case DATE:
+ return DateString.class;
+ case TIME:
+ return TimeString.class;
+ default:
+ throw new RuntimeException(
+ "Error deserializing range endpoint (did not find
compatible type)");
+ }
Review Comment:
`determineRangeEndpointValueClass` only supports a small subset of
`SqlTypeName` values (e.g., DECIMAL/DOUBLE/CHAR/BOOLEAN/TIMESTAMP/DATE/TIME).
Sarg ranges can be produced for other common types (e.g.,
INTEGER/BIGINT/SMALLINT/TINYINT/REAL/FLOAT/VARCHAR), and this method will throw
at runtime for them. Expand the mapping to cover the relevant Calcite literal
value representations (e.g., exact numerics -> BigDecimal, approximate ->
Double, character types -> NlsString, etc.).
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -923,6 +955,116 @@ RexNode toRex(RelInput relInput, Object o) {
}
}
+ private static <C extends Comparable<C>> Sarg<C> sargFromJson(Map<String,
Object> map, RelDataType type) {
+ final String nullAs = requireNonNull((String) map.get("nullAs"),
"nullAs");
+ final List<List<String>> rangeSet =
+ requireNonNull((List<List<String>>) map.get("rangeSet"),
"rangeSet");
+ return Sarg.of(ENUM_BY_NAME.get(nullAs),
+ RelJson.<C>rangeSetFromJson(rangeSet, type));
+ }
Review Comment:
Sarg serialization/deserialization is newly introduced here (`toJson(Sarg)`,
`sargFromJson`, `rangeSetFromJson`, `rangeFromJson`). There are existing
planner tests that exercise RelJson round-tripping; adding focused tests that
round-trip RexNodes containing Sarg/RangeSet (including different type families
and nullAs values) would help prevent regressions, especially around endpoint
typing and range shapes.
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -923,6 +955,116 @@ RexNode toRex(RelInput relInput, Object o) {
}
}
+ private static <C extends Comparable<C>> Sarg<C> sargFromJson(Map<String,
Object> map, RelDataType type) {
+ final String nullAs = requireNonNull((String) map.get("nullAs"),
"nullAs");
+ final List<List<String>> rangeSet =
+ requireNonNull((List<List<String>>) map.get("rangeSet"),
"rangeSet");
+ return Sarg.of(ENUM_BY_NAME.get(nullAs),
+ RelJson.<C>rangeSetFromJson(rangeSet, type));
+ }
+
+ /** Converts a JSON list to a {@link RangeSet} with supplied value typing.
*/
+ private static <C extends Comparable<C>> RangeSet<C> rangeSetFromJson(
+ List<List<String>> rangeSetsJson, RelDataType type) {
+ final ImmutableRangeSet.Builder<C> builder =
ImmutableRangeSet.builder();
+ try {
+ rangeSetsJson.forEach(list -> builder.add(rangeFromJson(list,
type)));
+ } catch (Exception e) {
+ throw new RuntimeException("Error creating RangeSet from JSON: ",
e);
+ }
+ return builder.build();
+ }
+
+ /**
+ * Creates a {@link Range} from a JSON object.
+ *
+ * <p>The JSON object is as serialized using {@link #toJson(Range)},
+ * e.g. {@code ["[", ")", 10, "-"]}.
+ */
+ private static <C extends Comparable<C>> Range<C> rangeFromJson(
+ List<String> list, RelDataType type) {
+ switch (list.get(0)) {
+ case "all":
+ return Range.all();
+ case "atLeast":
+ return Range.atLeast(rangeEndPointFromJson(list.get(1), type));
+ case "atMost":
+ return Range.atMost(rangeEndPointFromJson(list.get(1), type));
+ case "greaterThan":
+ return Range.greaterThan(rangeEndPointFromJson(list.get(1),
type));
+ case "lessThan":
+ return Range.lessThan(rangeEndPointFromJson(list.get(1),
type));
+ case "singleton":
+ return Range.singleton(rangeEndPointFromJson(list.get(1),
type));
+ case "closed":
+ return Range.closed(rangeEndPointFromJson(list.get(1), type),
+ rangeEndPointFromJson(list.get(2), type));
+ case "closedOpen":
+ return Range.closedOpen(rangeEndPointFromJson(list.get(1)),
+ rangeEndPointFromJson(list.get(2), type));
+ case "openClosed":
+ return Range.openClosed(rangeEndPointFromJson(list.get(1)),
Review Comment:
In `rangeFromJson`, the `closedOpen` branch uses the deprecated
`rangeEndPointFromJson(Object)` overload for the lower endpoint
(`rangeEndPointFromJson(list.get(1))`), while other branches use the typed
overload that considers `RelDataType`. This looks like an accidental mismatch
and can lead to inconsistent endpoint typing; use the `(Object, RelDataType)`
overload for both endpoints.
```suggestion
return Range.closedOpen(rangeEndPointFromJson(list.get(1),
type),
rangeEndPointFromJson(list.get(2), type));
case "openClosed":
return Range.openClosed(rangeEndPointFromJson(list.get(1),
type),
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -923,6 +955,116 @@ RexNode toRex(RelInput relInput, Object o) {
}
}
+ private static <C extends Comparable<C>> Sarg<C> sargFromJson(Map<String,
Object> map, RelDataType type) {
+ final String nullAs = requireNonNull((String) map.get("nullAs"),
"nullAs");
+ final List<List<String>> rangeSet =
+ requireNonNull((List<List<String>>) map.get("rangeSet"),
"rangeSet");
+ return Sarg.of(ENUM_BY_NAME.get(nullAs),
+ RelJson.<C>rangeSetFromJson(rangeSet, type));
+ }
+
+ /** Converts a JSON list to a {@link RangeSet} with supplied value typing.
*/
+ private static <C extends Comparable<C>> RangeSet<C> rangeSetFromJson(
+ List<List<String>> rangeSetsJson, RelDataType type) {
+ final ImmutableRangeSet.Builder<C> builder =
ImmutableRangeSet.builder();
+ try {
+ rangeSetsJson.forEach(list -> builder.add(rangeFromJson(list,
type)));
+ } catch (Exception e) {
+ throw new RuntimeException("Error creating RangeSet from JSON: ",
e);
+ }
+ return builder.build();
+ }
+
+ /**
+ * Creates a {@link Range} from a JSON object.
+ *
+ * <p>The JSON object is as serialized using {@link #toJson(Range)},
+ * e.g. {@code ["[", ")", 10, "-"]}.
+ */
+ private static <C extends Comparable<C>> Range<C> rangeFromJson(
+ List<String> list, RelDataType type) {
+ switch (list.get(0)) {
+ case "all":
+ return Range.all();
+ case "atLeast":
+ return Range.atLeast(rangeEndPointFromJson(list.get(1), type));
+ case "atMost":
+ return Range.atMost(rangeEndPointFromJson(list.get(1), type));
+ case "greaterThan":
+ return Range.greaterThan(rangeEndPointFromJson(list.get(1),
type));
+ case "lessThan":
+ return Range.lessThan(rangeEndPointFromJson(list.get(1),
type));
+ case "singleton":
+ return Range.singleton(rangeEndPointFromJson(list.get(1),
type));
+ case "closed":
+ return Range.closed(rangeEndPointFromJson(list.get(1), type),
+ rangeEndPointFromJson(list.get(2), type));
+ case "closedOpen":
+ return Range.closedOpen(rangeEndPointFromJson(list.get(1)),
+ rangeEndPointFromJson(list.get(2), type));
+ case "openClosed":
+ return Range.openClosed(rangeEndPointFromJson(list.get(1)),
Review Comment:
In `rangeFromJson`, the `openClosed` branch also uses the deprecated
`rangeEndPointFromJson(Object)` overload for the lower endpoint. To keep
Sarg/RangeSet deserialization type-safe and consistent, use the `(Object,
RelDataType)` overload for both endpoints.
```suggestion
return Range.closedOpen(rangeEndPointFromJson(list.get(1),
type),
rangeEndPointFromJson(list.get(2), type));
case "openClosed":
return Range.openClosed(rangeEndPointFromJson(list.get(1),
type),
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
