You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2025/07/17 15:30:52 UTC
[PR] IGNITE-25716 Change default TIME dynamic parameter precision to 6 [ignite-3]
xtern opened a new pull request, #6274:
URL: https://github.com/apache/ignite-3/pull/6274
https://issues.apache.org/jira/browse/IGNITE-25716
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2222636581
##########
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:
the same as for time - why we need such a change ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2215845282
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -128,6 +128,7 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
public static final int MAX_LENGTH_OF_ALIASES = 256;
public static final int DECIMAL_DYNAMIC_PARAM_PRECISION = 28;
public static final int DECIMAL_DYNAMIC_PARAM_SCALE = 6;
+ private static final int TIME_DYNAMIC_PARAM_PRECISION = 6;
Review Comment:
why 6 and not 3 or 9 ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2221730953
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##########
@@ -68,12 +68,12 @@ static void initTestData() {
.app("CREATE TABLE DATE1 (pk DATE, val DATE, PRIMARY KEY USING SORTED (pk));").nl()
.app("CREATE TABLE DATE2 (pk DATE, val DATE, PRIMARY KEY USING HASH (pk));").nl()
.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 TIME2 (pk TIME(6), val TIME, PRIMARY KEY USING HASH (pk));").nl()
Review Comment:
I added this for consistency, for example if a dynamic parameter had a default precision of 0 (although for consistency it was probably necessary to set precision = 9).
Anyway I think you're right, we don't need it.
Reverted this change, 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2223040471
##########
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:
in such a case the same tests need for TIME, TS_WITH_LOCAL_TZ ? and seems PR and issue description changes
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2224474201
##########
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:
do we have a tests for such a case ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2221730953
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##########
@@ -68,12 +68,12 @@ static void initTestData() {
.app("CREATE TABLE DATE1 (pk DATE, val DATE, PRIMARY KEY USING SORTED (pk));").nl()
.app("CREATE TABLE DATE2 (pk DATE, val DATE, PRIMARY KEY USING HASH (pk));").nl()
.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 TIME2 (pk TIME(6), val TIME, PRIMARY KEY USING HASH (pk));").nl()
Review Comment:
I added this for consistency, for example if a dynamic parameter had a default precision of 0.
But you're right, we don't need it.
Reverted this change, 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2222661816
##########
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:
To check the issue with indexes and TIMESTAMP dynamic parameter.
When the column precision is lower than the dynamic parameter precision, indexes are not used now. This issue is fixed in this patch (see change in TypeUtils).
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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.120000000' - 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2221746467
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -128,6 +128,7 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
public static final int MAX_LENGTH_OF_ALIASES = 256;
public static final int DECIMAL_DYNAMIC_PARAM_PRECISION = 28;
public static final int DECIMAL_DYNAMIC_PARAM_SCALE = 6;
+ private static final int TIME_DYNAMIC_PARAM_PRECISION = 6;
Review Comment:
I changed precision to 9 (nanoseconds). 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2224427255
##########
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:
involved predicates can not match TableScanToKeyValueGetRule usage, thus it need to be removed if EQ predicates are not planned to use, otherwise it confused. Or probably it more clear to extend SORTED_INDEXES with "pkIndexName(table)" wdyt ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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.120000000' - 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2223034471
##########
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:
next cases are unclear for now : create table ... col TIME(3) and further query : select from .. where col = TIME '12:12:12.1234' is it need to be implicitly casted ? And on the contrary: create table ... col TIME(3) and further : select from .. where col = TIME '12:12:12.12'. Other words - temporal predicate with great or less precision than column is.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2215985352
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -128,6 +128,7 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
public static final int MAX_LENGTH_OF_ALIASES = 256;
public static final int DECIMAL_DYNAMIC_PARAM_PRECISION = 28;
public static final int DECIMAL_DYNAMIC_PARAM_SCALE = 6;
+ private static final int TIME_DYNAMIC_PARAM_PRECISION = 6;
Review Comment:
I set the value to `6` only because TIMESTAMP has a default precision of `6`.
Maybe it's worth setting the value to `9` (for the future). Because it doesn't affect any resources.
If no one is against it, I'll change it to `9` a little later.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2215845282
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/IgniteSqlValidator.java:
##########
@@ -128,6 +128,7 @@ public class IgniteSqlValidator extends SqlValidatorImpl {
public static final int MAX_LENGTH_OF_ALIASES = 256;
public static final int DECIMAL_DYNAMIC_PARAM_PRECISION = 28;
public static final int DECIMAL_DYNAMIC_PARAM_SCALE = 6;
+ private static final int TIME_DYNAMIC_PARAM_PRECISION = 6;
Review Comment:
why 6 and not 3 (calcite limitation) or 9 (nanoseconds) ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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.120000000' - 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2222661816
##########
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:
To check the issue with indexes and TIMESTAMP dynamic parameter.
When the TIMESTAMP column precision is lower than the dynamic parameter precision, indexes are not used now. This issue is fixed in this patch (see change in TypeUtils).
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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, 86400000L), 86400000L)), 86400000L), 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change the default precision of temporal dynamic parameter to 9 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2222661816
##########
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:
To check the issue with indexes and TIMESTAMP dynamic parameter.
When the column precision is lower than the dynamic parameter precision, indexes are not used now. This issue is fixed in this patch (see change in RexUtils).
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 6 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2221601766
##########
modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java:
##########
@@ -68,12 +68,12 @@ static void initTestData() {
.app("CREATE TABLE DATE1 (pk DATE, val DATE, PRIMARY KEY USING SORTED (pk));").nl()
.app("CREATE TABLE DATE2 (pk DATE, val DATE, PRIMARY KEY USING HASH (pk));").nl()
.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 TIME2 (pk TIME(6), val TIME, PRIMARY KEY USING HASH (pk));").nl()
Review Comment:
why do we need such a change ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25716 Change TIME dynamic parameter default precision to 9 [ignite-3]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #6274:
URL: https://github.com/apache/ignite-3/pull/6274#discussion_r2224473433
##########
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 ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org