Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern merged PR #6274: URL: https://github.com/apache/ignite-3/pull/6274 -- 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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method `toTimeExact(int, int)`, sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment
:thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method `toTimeExact(int, int)`, sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
This happens because RexImpTable#normalize calls built-in method
`FLOOR_MOD(Math.class, "floorMod", long.class, long.class)`. And return value
is long.
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment
:thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method `toTimeExact(int, int)`, sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
The minor issue here - Math.floorMod(long, long) is called instead of
floorMod(int, int).
This happens because RexImpTable#normalize calls built-in method
`FLOOR_MOD(Math.class, "floorMod", long.class, long.class)` . And return value
is long.
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment
:thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method `toTimeExact(int, int)`, sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
The minor issue here - Math.floorMod(long, long) is called instead of
floorMod(int, int).
This happens because RexImpTable#normalize calls method
`FLOOR_MOD(Math.class, "floorMod", long.class, long.class)` . And return value
is long.
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment
:thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method `toTimeExact(int, int)`, sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
The minor issue here - Math.floorMod(long, long) is called instead of
floorMod(int, int).
This happens because RexImpTable#normalize calls method
`FLOOR_MOD(Math.class, "floorMod", long.class, long.class)` . And return value
is long.
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment.
Another way - I can introduce our own Ignite method `floorMod(int, int)` and
use it in `normalize` :thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239182369
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I was thinking about another method (int, int), sorry for the
misunderstanding.
This method (long, int) is called during interval arithmetic.
```sql
SELECT ? + INTERVAL 1 DAY
```
We generate something like this
```java
outBuilder.addField(value_dynamic_param == null ? 0 :
IgniteSqlFunctions.toTimeExact(Math.floorMod(IgniteMath.addExact(value_dynamic_param.intValue(),
(int) Math.floorMod(IgniteMath.multiplyExact(1, 8640L), 8640L)),
8640L), 6));
```
The minor issue here - Math.floorMod(long, long) is called instead of
floorMod(int, int).
This happens because RexImpTable#normalize calls method
`FLOOR_MOD(Math.class, "floorMod", long.class, long.class)` . And return value
is long.
I couldn't reproduce the case where `toTimeExact(long, int)` needed to
adjust fractions of a second, but for consistency I kept that adjustment.
Another way - I can introduce our own Ignite method `floorMod(int, int)` and
use it in `normalize` :thinking:
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2239097986
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
Fixed, thanks.
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
lowka commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2236404249
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
>i see that we can avoid unboxing here, isn`t it ?
Untyped variants of `toTimeExact` are used to for dynamic parameters, and
types of dynamic parameters are always nullable.
So, no you can not avoid boxing here. see See RexToLitTranslator
visitDynamicParam. Code to retrieve dynamic parameter `N` looks like this:
```
Object value = ctx.get("?N");
```
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
>i see that we can avoid unboxing here, isn`t it ?
Untyped variants of `toTimeExact` are used to for dynamic parameters, and
types of dynamic parameters are always nullable.
So, no you can not avoid boxing here. see See RexToLitTranslator
visitDynamicParam. Code to retrieve dynamic parameter `N` looks like this:
```
Object value = ctx.get("?N");
```
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2236061277
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
I run ItTemporalIndexTest#testTemporalPrecisionLookupDynamicParam as you
suggested under debug and seems this code is not involved, did i miss smth ?
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2236043336
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
you cast Integer to int here for further usage in
IgniteSqlDateTimeUtils.adjustTimeMillis(@Nullable Integer time
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2231271014
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/RexUtils.java:
##
@@ -1325,6 +1325,12 @@ public static boolean isLosslessCast(RexNode node) {
&& (source.getPrecision() <= target.getPrecision() ||
target.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED);
}
+// TIME, TIMESTAMP and TIMESTAMP_WLTZ can use index, ignoring
precision.
+if (source.getSqlTypeName() == target.getSqlTypeName()
Review Comment:
Thanks, I believe I fixed it.
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2231275163
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java:
##
@@ -643,6 +643,12 @@ public static boolean
needCastInSearchBounds(IgniteTypeFactory typeFactory, RelD
return false;
}
+// TIME, TIMESTAMP and TIMESTAMP_WLTZ can use index, ignoring
precision.
+if (fromType.getSqlTypeName() == toType.getSqlTypeName()
+&& (fromType.getSqlTypeName() == SqlTypeName.TIME ||
SqlTypeUtil.isTimestamp(fromType))) {
Review Comment:
Done, thanks
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2231271014
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/RexUtils.java:
##
@@ -1325,6 +1325,12 @@ public static boolean isLosslessCast(RexNode node) {
&& (source.getPrecision() <= target.getPrecision() ||
target.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED);
}
+// TIME, TIMESTAMP and TIMESTAMP_WLTZ can use index, ignoring
precision.
+if (source.getSqlTypeName() == target.getSqlTypeName()
Review Comment:
Thanks, I believe I've fixed it.
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
korlov42 commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230544072
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java:
##
@@ -643,6 +643,12 @@ public static boolean
needCastInSearchBounds(IgniteTypeFactory typeFactory, RelD
return false;
}
+// TIME, TIMESTAMP and TIMESTAMP_WLTZ can use index, ignoring
precision.
+if (fromType.getSqlTypeName() == toType.getSqlTypeName()
+&& (fromType.getSqlTypeName() == SqlTypeName.TIME ||
SqlTypeUtil.isTimestamp(fromType))) {
Review Comment:
```suggestion
&& SqlTypeUtil.isDatetime(fromType)) {
```
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/RexUtils.java:
##
@@ -1325,6 +1325,12 @@ public static boolean isLosslessCast(RexNode node) {
&& (source.getPrecision() <= target.getPrecision() ||
target.getPrecision() == RelDataType.PRECISION_NOT_SPECIFIED);
}
+// TIME, TIMESTAMP and TIMESTAMP_WLTZ can use index, ignoring
precision.
+if (source.getSqlTypeName() == target.getSqlTypeName()
Review Comment:
This breaks the contract of the method. The cast between temporal types
lossless iff target precision >= source precision and this is the cast within
the same type (perhaps, some casts from time to timestamp or from date to
timestamp are lossless as well, but I would not add this for now)
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230342452
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
Thanks, now we have such tests
See `temporalPrecisionLookupDynParamArgs` test cases with explicit dyn param
cast e.g. ` = ?::TIME(X)`
If we skip conversion of dyn param value in
RexToLuxTranslator#visitDynamicParam then the value remains unchanged and we
will miss the value in the index.
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274: URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230329217 ## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java: ## @@ -598,6 +598,12 @@ public static boolean needCastInSearchBounds(IgniteTypeFactory typeFactory, RelD return false; } +// TIME, TIMESTAMP and TIMESTAMP_LTZ can use index, ignoring precision. +if (fromType.getSqlTypeName() == toType.getSqlTypeName() Review Comment: For example, if we have a VARCHAR(3) column with the row 'abc' and in a query we search for 'abcd', I'd be surprised if we find 'abc' when we search for 'abcd' without explicit casting. And vice versa. It doesn't meter do we search '00:00:00.12' or '00:00:00.12000' - this is the same value of time The following tests have been added to check for correctness: testTemporalPrecisionLookupDynamicParam testTemporalPrecisionLookupLiteral testTemporalPrecisionGreaterLowerDynamicParam testTemporalPrecisionGreaterLowerLiteral -- 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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274: URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230329217 ## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java: ## @@ -598,6 +598,12 @@ public static boolean needCastInSearchBounds(IgniteTypeFactory typeFactory, RelD return false; } +// TIME, TIMESTAMP and TIMESTAMP_LTZ can use index, ignoring precision. +if (fromType.getSqlTypeName() == toType.getSqlTypeName() Review Comment: For example, if we have a VARCHAR(3) column with the row 'abc' and in a query we search for 'abcd', I'd be surprised if we find 'abc' when we search for 'abcd' without explicit casting. And vice versa. It doesn't meter do we search '00:00:00.12' or '00:00:00.12000' - this is the same value of time The following tests have been added to check for correctness: testTemporalLookupPrecisionDynamicParam testTemporalLookupPrecisionLiteral testTemporalPrecisionGreaterLowerDynamicParam testTemporalPrecisionGreaterLowerLiteral -- 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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230301275
##
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##
@@ -70,10 +70,10 @@ static void initTestData() {
.app("CREATE TABLE TIME1 (pk TIME, val TIME, PRIMARY KEY USING
SORTED (pk));").nl()
.app("CREATE TABLE TIME2 (pk TIME, val TIME, PRIMARY KEY USING
HASH (pk));").nl()
.app("CREATE TABLE TIMESTAMP1 (pk TIMESTAMP, val TIMESTAMP,
PRIMARY KEY USING SORTED (pk));").nl()
-.app("CREATE TABLE TIMESTAMP2 (pk TIMESTAMP, val TIMESTAMP,
PRIMARY KEY USING HASH (pk));").nl()
+.app("CREATE TABLE TIMESTAMP2 (pk TIMESTAMP(0), val TIMESTAMP,
PRIMARY KEY USING HASH (pk));").nl()
Review Comment:
thanks, change reverted, PR description updated
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230344893
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
Could you clarify your suggestion?
Are you talking about removing `@Nullable Integer toTimeExact(@Nullable
Object object`?
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230344893
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
Review Comment:
Can you clarify your suggestion?
Are you talking about removing `@Nullable Integer toTimeExact(@Nullable
Object object`?
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230342452
##
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/IgniteSqlFunctions.java:
##
@@ -362,8 +363,33 @@ public static BigDecimal toBigDecimal(Number value, int
precision, int scale) {
}
}
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static @Nullable Integer toTimeExact(@Nullable Object object, int
precision) {
+if (object == null) {
+return null;
+}
+
+assert object instanceof Integer : object.getClass();
+
+return toTimeExact((int) object, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(int val, int precision) {
+assert precision >= 0 : "Invalid precision: " + precision;
+
+return IgniteSqlDateTimeUtils.adjustTimeMillis(val, precision);
+}
+
+/** Adjusts precision of {@link SqlTypeName#TIME} value. */
+public static int toTimeExact(long val, int precision) {
Review Comment:
Thanks, now we have such tests
See `temporalPrecisionLookupDynParamArgs` test cases with explicit dyn param
cast e.g. ` = ?::TIME(X)`
If we skip conversion of dyn param value in
RexToLuxTranslator#visitDynamicParam then the value remains unchanged.
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230331093
##
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##
@@ -158,6 +181,27 @@ public void testSearchGtLtWithPkIdxUsage(String table,
String predicate, Object
}
}
+/** Check range predicates with dynamic parameter. */
+@ParameterizedTest(name = "table = {0}, predicate = {1}")
+@MethodSource("geLeSearchDynParamArguments")
+public void testSearchGtLtWithPkIdxUsageDynamicParam(String table, String
predicate, Object parameter, Object result) {
+assertQuery(format("SELECT /*+ FORCE_INDEX({}),
DISABLE_RULE('TableScanToKeyValueGetRule') */ val FROM {} WHERE pk {} ORDER BY
val",
Review Comment:
Thanks, Now in these tests I check all indexes in one loop (if I understood
your suggestion correctly).
--
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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274: URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230329217 ## modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/TypeUtils.java: ## @@ -598,6 +598,12 @@ public static boolean needCastInSearchBounds(IgniteTypeFactory typeFactory, RelD return false; } +// TIME, TIMESTAMP and TIMESTAMP_LTZ can use index, ignoring precision. +if (fromType.getSqlTypeName() == toType.getSqlTypeName() Review Comment: For example, if we set a VARCHAR(3) column with the row 'abc' and in a query we search for 'abcd', I'd be surprised if we find 'abc' when we search for 'abcd' without explicit casting. And vice versa. It doesn't meter do we search '00:00:00.12' or '00:00:00.12000' - this is the same value of time The following tests have been added to check for correctness: testTemporalLookupPrecisionDynamicParam testTemporalLookupPrecisionLiteral testTemporalPrecisionGreaterLowerDynamicParam testTemporalPrecisionGreaterLowerLiteral -- 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]
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2230301275
##
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##
@@ -70,10 +70,10 @@ static void initTestData() {
.app("CREATE TABLE TIME1 (pk TIME, val TIME, PRIMARY KEY USING
SORTED (pk));").nl()
.app("CREATE TABLE TIME2 (pk TIME, val TIME, PRIMARY KEY USING
HASH (pk));").nl()
.app("CREATE TABLE TIMESTAMP1 (pk TIMESTAMP, val TIMESTAMP,
PRIMARY KEY USING SORTED (pk));").nl()
-.app("CREATE TABLE TIMESTAMP2 (pk TIMESTAMP, val TIMESTAMP,
PRIMARY KEY USING HASH (pk));").nl()
+.app("CREATE TABLE TIMESTAMP2 (pk TIMESTAMP(0), val TIMESTAMP,
PRIMARY KEY USING HASH (pk));").nl()
Review Comment:
change reverted
--
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]
