alex-plekhanov commented on a change in pull request #9826:
URL: https://github.com/apache/ignite/pull/9826#discussion_r826713933
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -887,7 +888,7 @@ private DecimalMinMax(boolean min) {
}
/** */
- private static class VarBinaryMinMax<T extends Comparable<T>> implements
Accumulator {
+ private static class VarBinaryMinMax implements Accumulator {
Review comment:
Let's use something like this:
```
/** */
private static class ComparableMinMax<T extends Comparable<T>>
implements Accumulator {
/** */
public static final Supplier<Accumulator> VARBINARY_MIN_FACTORY = ()
-> new ComparableMinMax<ByteString>(true,
tf -> tf.createTypeWithNullability(tf.createSqlType(VARBINARY),
true));
/** */
public static final Supplier<Accumulator> VARBINARY_MAX_FACTORY = ()
-> new ComparableMinMax<ByteString>(false,
tf -> tf.createTypeWithNullability(tf.createSqlType(VARBINARY),
true));
/** */
public static final Supplier<Accumulator> UUID_MIN_FACTORY = () ->
new ComparableMinMax<UUID>(true,
tf -> tf.createTypeWithNullability(tf.createUUIDType(), true));
/** */
public static final Supplier<Accumulator> UUID_MAX_FACTORY = () ->
new ComparableMinMax<UUID>(false,
tf -> tf.createTypeWithNullability(tf.createUUIDType(), true));
/** */
private final boolean min;
/** */
private final Function<IgniteTypeFactory, RelDataType> typeSupplier;
/** */
private T val;
/** */
private boolean empty = true;
/** */
private ComparableMinMax(boolean min, Function<IgniteTypeFactory,
RelDataType> typeSupplier) {
this.min = min;
this.typeSupplier = typeSupplier;
}
/** {@inheritDoc} */
@Override public void add(Object... args) {
T in = (T)args[0];
if (in == null)
return;
val = empty ? in : min ?
(val.compareTo(in) < 0 ? val : in) :
(val.compareTo(in) < 0 ? in : val);
empty = false;
}
/** {@inheritDoc} */
@Override public void apply(Accumulator other) {
ComparableMinMax<T> other0 = (ComparableMinMax<T>)other;
if (other0.empty)
return;
val = empty ? other0.val : min ?
(val.compareTo(other0.val) < 0 ? val : other0.val) :
(val.compareTo(other0.val) < 0 ? other0.val : val);
empty = false;
}
/** {@inheritDoc} */
@Override public Object end() {
return empty ? null : val;
}
/** {@inheritDoc} */
@Override public List<RelDataType> argumentTypes(IgniteTypeFactory
typeFactory) {
return F.asList(typeSupplier.apply(typeFactory));
}
/** {@inheritDoc} */
@Override public RelDataType returnType(IgniteTypeFactory
typeFactory) {
return typeSupplier.apply(typeFactory);
}
}
```
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -109,7 +110,7 @@
case ANY:
if (call.type instanceof UuidType)
Review comment:
Do we really need check for UuidType? Let's throw an exception for any
`ANY` type
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/IgniteTypeFactory.java
##########
@@ -259,11 +266,26 @@ else if (type instanceof BasicSqlType || type instanceof
IntervalSqlType) {
return
createTypeWithNullability(createSqlIntervalType(INTERVAL_QUALIFIER_DAY_TIME),
true);
else if (clazz == Period.class)
return
createTypeWithNullability(createSqlIntervalType(INTERVAL_QUALIFIER_YEAR_MONTH),
true);
+ else if (clazz == UUID.class)
+ return createTypeWithNullability(createUUIDType(), true);
}
return super.toSql(type);
}
+ /** @return UUID SQL type. */
+ public RelDataType createUUIDType() {
Review comment:
Camel case
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java
##########
@@ -379,4 +381,16 @@ private static long fromLocalTs(DataContext ctx, long ts) {
// Taking into account DST, offset can be changed after converting
from UTC to time-zone.
return ts - tz.getOffset(ts - tz.getOffset(ts));
}
+
+ /**
+ * @return RexLiteral
+ */
+ public static RexNode toRexLiteral(Object dfltVal, IgniteTypeFactory
typeFactory, RexBuilder rexBuilder,
+ ColumnDescriptor desc) {
Review comment:
Utility class should not depend on `ColumnDescriptor`, it looks strange.
Let's simplify a little bit, for example, like this:
```
/**
* @return RexNode
*/
public static RexNode toRexLiteral(RexBuilder rexBuilder, DataContext
ctx, Object dfltVal, RelDataType type) {
if (dfltVal instanceof UUID) {
// There is no internal UUID data type in Calcite, so convert
UUID to VARCHAR literal and then cast to UUID.
dfltVal = dfltVal.toString();
}
else
dfltVal = toInternal(ctx, dfltVal);
return rexBuilder.makeLiteral(dfltVal, type, true);
}
```
And caller:
```
return TypeUtils.toRexLiteral(rexBuilder, dataCtx,
desc.defaultValue(), desc.logicalType(typeFactory));
```
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java
##########
@@ -379,4 +381,16 @@ private static long fromLocalTs(DataContext ctx, long ts) {
// Taking into account DST, offset can be changed after converting
from UTC to time-zone.
return ts - tz.getOffset(ts - tz.getOffset(ts));
}
+
+ /**
+ * @return RexLiteral
+ */
+ public static RexNode toRexLiteral(Object dfltVal, IgniteTypeFactory
typeFactory, RexBuilder rexBuilder,
+ ColumnDescriptor desc) {
+ if (dfltVal instanceof UUID)
+ return rexBuilder.makeCast(typeFactory.createUUIDType(),
Review comment:
`desc.logicalType(typeFactory)` - is already `UuidType`. We should make
`varchar` literal and than cast it to uuid.
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -89,7 +90,7 @@
case INTEGER:
case ANY:
Review comment:
Wrong place for case (should be out of double, real, float and integer
scope)
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -109,7 +110,7 @@
case ANY:
if (call.type instanceof UuidType)
- return () -> new Sum(new UuidSumEmptyIsZero());
+ throw new UnsupportedOperationException("SUM() is not
supported for UUID type.");
Review comment:
As far as I understand `sumEmptyIsZeroFactory` should be covered too.
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -89,7 +90,7 @@
case INTEGER:
case ANY:
if (call.type instanceof UuidType)
Review comment:
Do we really need check for UuidType? Let's throw an exception for any
`ANY` 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]
