zstan commented on code in PR #13569:
URL: https://github.com/apache/ignite/pull/13569#discussion_r4014589894
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/OtherType.java:
##########
@@ -29,21 +28,12 @@ public OtherType(boolean nullable) {
/** {@inheritDoc} */
@Override protected void generateTypeString(StringBuilder sb, boolean
withDetail) {
- sb.append("OTHER");
+ // The digest must differ from Calcite's OTHER to keep the types
distinct in its shared type cache.
+ sb.append(withDetail ? "IGNITE_OTHER" : "OTHER");
Review Comment:
can we use simple : sb.append("IGNITE_OTHER"); here ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ReflectiveCallNotNullImplementor.java:
##########
@@ -66,6 +68,24 @@ public ReflectiveCallNotNullImplementor(Method method) {
callExpr = Expressions.call(target, method, translatedOperands);
}
+
+ if (TypeUtils.isConvertableType(method.getReturnType())) {
+ Type targetType =
translator.typeFactory.getJavaClass(call.getType());
+ Expression converted = ConverterUtils.toInternal(callExpr,
targetType);
+
+ if (converted != callExpr)
+ callExpr = converted;
+ else {
+ Expression result = method.getReturnType().isPrimitive() ?
Expressions.box(callExpr) : callExpr;
Review Comment:
dead code ? CONVERTABLE_TYPES has no primitives
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/ConverterUtils.java:
##########
@@ -139,12 +149,32 @@ static List<Expression> fromInternal(Class<?>[]
targetTypes,
else
type = targetTypes[j].getComponentType();
- list.add(fromInternal(expressions.get(i), type));
+ list.add(fromInternal(root, expressions.get(i), type));
}
}
return list;
}
+ /** */
+ private static Expression fromInternal(@Nullable Expression root,
Expression operand, Type targetType) {
+ // Preserve Calcite's calendar conversion for JDBC dates and
timestamps.
+ Expression converted = fromInternal(operand, targetType);
+
+ if (root == null || converted != operand ||
!TypeUtils.isConvertableType(targetType))
+ return converted;
+
+ if (Types.isAssignableFrom(targetType, operand.getType()))
+ return operand;
Review Comment:
```suggestion
if (Types.isAssignableFrom(targetType, operand.getType()))
return operand;
Expression converted = fromInternal(operand, targetType);
if (root == null || converted != operand ||
!TypeUtils.isConvertableType(targetType))
return converted;
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -409,23 +415,39 @@ else if (val instanceof Number && storageType !=
val.getClass()) {
* @return Millis value.
*/
private static long toLong(DataContext ctx, Object val) {
+ // Java time values have no time zone and use the proleptic Gregorian
calendar, as does Calcite.
if (val instanceof LocalDateTime)
- return
toLong(DateValueUtils.convertToTimestamp((LocalDateTime)val),
DataContext.Variable.TIME_ZONE.get(ctx));
+ return
((LocalDateTime)val).toInstant(ZoneOffset.UTC).toEpochMilli();
if (val instanceof LocalDate)
- return toLong(DateValueUtils.convertToSqlDate((LocalDate)val),
DataContext.Variable.TIME_ZONE.get(ctx));
+ return ((LocalDate)val).toEpochDay() *
DateTimeUtils.MILLIS_PER_DAY;
if (val instanceof LocalTime)
- return toLong(DateValueUtils.convertToSqlTime((LocalTime)val),
DataContext.Variable.TIME_ZONE.get(ctx));
+ return
TimeUnit.NANOSECONDS.toMillis(((LocalTime)val).toNanoOfDay());
- return toLong((java.util.Date)val,
DataContext.Variable.TIME_ZONE.get(ctx));
+ return toLong((java.util.Date)val, timeZone(ctx));
}
/** */
private static long toLong(java.util.Date val, TimeZone tz) {
long time = val.getTime();
+ long locTs = time + tz.getOffset(time);
+
+ if (locTs >= GREGORIAN_CUTOVER)
Review Comment:
I just try to know - why do we need it , comment this code and all tests
from JdbcTestSuite are passed, plz clarify why we need it ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -83,6 +85,10 @@
/** */
public class TypeUtils {
+ /** Start of the Gregorian part of the calendar used by JDBC temporal
types. */
+ private static final long GREGORIAN_CUTOVER =
Review Comment:
I wonder, i can\`t see any changes in jdbc tests but they are mentioned, plz
clarify - why do we need such a change ?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtilsTest.java:
##########
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.processors.query.calcite.util;
+
+import java.lang.reflect.Type;
+import java.sql.Date;
+import java.sql.Time;
+import java.sql.Timestamp;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.LocalTime;
+import java.util.Calendar;
+import java.util.Collections;
+import java.util.GregorianCalendar;
+import java.util.Locale;
+import java.util.TimeZone;
+import org.apache.calcite.DataContext;
+import org.apache.calcite.DataContexts;
+import org.apache.calcite.util.DateString;
+import org.apache.calcite.util.TimeString;
+import org.apache.calcite.util.TimestampString;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+/** */
Review Comment:
Plz append minimal description, it\`s not clear a mission of such a test
class.
--
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]
