xtern commented on code in PR #5517: URL: https://github.com/apache/ignite-3/pull/5517#discussion_r2020815627
########## modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java: ########## @@ -0,0 +1,324 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.ignite.internal.sql.engine; + +import static java.time.format.DateTimeFormatter.ISO_LOCAL_TIME; +import static org.apache.ignite.internal.catalog.commands.CatalogUtils.pkIndexName; +import static org.apache.ignite.internal.lang.IgniteStringFormatter.format; +import static org.apache.ignite.internal.sql.engine.util.QueryChecker.containsIndexScan; +import static org.apache.ignite.internal.sql.engine.util.QueryChecker.matchesOnce; +import static org.apache.ignite.internal.sql.engine.util.SqlTestUtils.SQL_CONFORMANT_DATETIME_FORMATTER; + +import java.time.LocalDate; +import java.time.LocalDateTime; +import java.time.LocalTime; +import java.util.List; +import java.util.Map; +import java.util.TimeZone; +import java.util.function.Function; +import java.util.stream.Stream; +import org.apache.ignite.internal.lang.IgniteStringBuilder; +import org.apache.ignite.internal.sql.BaseSqlIntegrationTest; +import org.apache.ignite.internal.sql.engine.util.QueryChecker; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** Integration tests check temporal indexes. */ +public class ItTemporalIndexTest extends BaseSqlIntegrationTest { + private static final LocalDate INITIAL_DATE = LocalDate.of(1552, 10, 1); + private static final LocalTime INITIAL_TIME = LocalTime.of(0, 0, 0); + private static final LocalDateTime INITIAL_TS = LocalDateTime.of(1552, 10, 1, 0, 0); + private static final TimeZone tz = TimeZone.getTimeZone("UTC"); + private static final Map<String, String[]> All_INDEXES = Map.of( + "DATE1", new String[]{"S_ASC_IDX_DATE1", "S_DESC_IDX_DATE1", "HASH_IDX_DATE1"}, + "TIME1", new String[]{"S_ASC_IDX_TIME1", "S_DESC_IDX_TIME1", "HASH_IDX_TIME1"}, + "TIMESTAMP1", new String[]{"S_ASC_IDX_TIMESTAMP1", "S_DESC_IDX_TIMESTAMP1", "HASH_IDX_TIMESTAMP1"}, + "TIMESTAMPTZ1", new String[]{"S_ASC_IDX_TZ1", "S_DESC_IDX_TZ1", "HASH_IDX_TZ1"} + ); + + private static final Map<String, String[]> SORTED_INDEXES = Map.of( + "DATE1", new String[]{"S_ASC_IDX_DATE1", "S_DESC_IDX_DATE1"}, + "TIME1", new String[]{"S_ASC_IDX_TIME1", "S_DESC_IDX_TIME1"}, + "TIMESTAMP1", new String[]{"S_ASC_IDX_TIMESTAMP1", "S_DESC_IDX_TIMESTAMP1"}, + "TIMESTAMPTZ1", new String[]{"S_ASC_IDX_TZ1", "S_DESC_IDX_TZ1"} + ); + + @BeforeAll + static void initTestData() { + String query = new IgniteStringBuilder() + .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 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 TIMESTAMPTZ1 (pk TIMESTAMP WITH LOCAL TIME ZONE, val TIMESTAMP WITH LOCAL TIME ZONE, " + + "PRIMARY KEY USING SORTED (pk));").nl() + .app("CREATE TABLE TIMESTAMPTZ2 (pk TIMESTAMP WITH LOCAL TIME ZONE, val TIMESTAMP WITH LOCAL TIME ZONE, " + + "PRIMARY KEY USING HASH (pk));").nl() + + .app("CREATE INDEX s_asc_idx_date1 ON date1 USING SORTED (val ASC);") + .app("CREATE INDEX s_desc_idx_date1 ON date1 USING SORTED (val DESC);") + .app("CREATE INDEX hash_idx_date1 ON date1 USING HASH (val);") + + .app("CREATE INDEX s_asc_idx_time1 ON time1 USING SORTED (val ASC);") + .app("CREATE INDEX s_desc_idx_time1 ON time1 USING SORTED (val DESC);") + .app("CREATE INDEX hash_idx_time1 ON time1 USING HASH (val);") + + .app("CREATE INDEX s_asc_idx_timestamp1 ON timestamp1 USING SORTED (val ASC);") + .app("CREATE INDEX s_desc_idx_timestamp1 ON timestamp1 USING SORTED (val DESC);") + .app("CREATE INDEX hash_idx_timestamp1 ON timestamp1 USING HASH (val);") + + .app("CREATE INDEX s_asc_idx_tz1 ON timestamptz1 USING SORTED (val ASC);") + .app("CREATE INDEX s_desc_idx_tz1 ON timestamptz1 USING SORTED (val DESC);") + .app("CREATE INDEX hash_idx_tz1 ON timestamptz1 USING HASH (val);") + + .toString(); + + sqlScript(query); + + TimeZone.setDefault(tz); Review Comment: Personally I don't like that we changing default timezone here, at least we should restore previous default after tests. But I think it's better to introduce separate `Instant` result for TIMESTAMP WITH LOCAL TIME ZONE type. ``` private static final Instant INITIAL_TS_LTZ = INITIAL_TS.toInstant(ZoneOffset.UTC); ``` and fix timezone_ltz related parts (see attached patch) ``` Index: modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java --- a/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java (revision 7f3c64db5abdb3626112cf47f64c87d21e4aa263) +++ b/modules/sql-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItTemporalIndexTest.java (date 1743417641370) @@ -24,12 +24,13 @@ import static org.apache.ignite.internal.sql.engine.util.QueryChecker.matchesOnce; import static org.apache.ignite.internal.sql.engine.util.SqlTestUtils.SQL_CONFORMANT_DATETIME_FORMATTER; +import java.time.Instant; import java.time.LocalDate; import java.time.LocalDateTime; import java.time.LocalTime; +import java.time.ZoneOffset; import java.util.List; import java.util.Map; -import java.util.TimeZone; import java.util.function.Function; import java.util.stream.Stream; import org.apache.ignite.internal.lang.IgniteStringBuilder; @@ -45,7 +46,8 @@ private static final LocalDate INITIAL_DATE = LocalDate.of(1552, 10, 1); private static final LocalTime INITIAL_TIME = LocalTime.of(0, 0, 0); private static final LocalDateTime INITIAL_TS = LocalDateTime.of(1552, 10, 1, 0, 0); - private static final TimeZone tz = TimeZone.getTimeZone("UTC"); + private static final Instant INITIAL_TS_LTZ = INITIAL_TS.toInstant(ZoneOffset.UTC); + private static final Map<String, String[]> All_INDEXES = Map.of( "DATE1", new String[]{"S_ASC_IDX_DATE1", "S_DESC_IDX_DATE1", "HASH_IDX_DATE1"}, "TIME1", new String[]{"S_ASC_IDX_TIME1", "S_DESC_IDX_TIME1", "HASH_IDX_TIME1"}, @@ -94,8 +96,6 @@ sqlScript(query); - TimeZone.setDefault(tz); - fillData("DATE1"); fillData("DATE2"); @@ -222,14 +222,14 @@ INITIAL_TS.plusSeconds(2) }), - Arguments.of("TIMESTAMPTZ1", " BETWEEN TIMESTAMP '" + Arguments.of("TIMESTAMPTZ1", " BETWEEN TIMESTAMP WITH LOCAL TIME ZONE '" + INITIAL_TS.plusSeconds(1).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "' AND TIMESTAMP '" + + "' AND TIMESTAMP WITH LOCAL TIME ZONE '" + INITIAL_TS.plusSeconds(2).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", new Object[] { - INITIAL_TS.plusSeconds(1).atZone(tz.toZoneId()).toInstant(), - INITIAL_TS.plusSeconds(2).atZone(tz.toZoneId()).toInstant() + INITIAL_TS_LTZ.plusSeconds(1), + INITIAL_TS_LTZ.plusSeconds(2) }) ); } @@ -250,18 +250,19 @@ + "'", INITIAL_TS.plusSeconds(9)), Arguments.of("TIMESTAMP1", " >= TIMESTAMP '" + INITIAL_TS.plusSeconds(9).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS.plusSeconds(9)), + Arguments.of("TIMESTAMP1", " < TIMESTAMP '" + INITIAL_TS.plusSeconds(1).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS), Arguments.of("TIMESTAMP1", " <= TIMESTAMP '" + INITIAL_TS.format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS), - Arguments.of("TIMESTAMPTZ1", " > TIMESTAMP '" + INITIAL_TS.plusSeconds(8).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.plusSeconds(9).atZone(tz.toZoneId()).toInstant()), - Arguments.of("TIMESTAMPTZ1", " >= TIMESTAMP '" + INITIAL_TS.plusSeconds(9).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.plusSeconds(9).atZone(tz.toZoneId()).toInstant()), - Arguments.of("TIMESTAMPTZ1", " < TIMESTAMP '" + INITIAL_TS.plusSeconds(1).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.atZone(tz.toZoneId()).toInstant()), - Arguments.of("TIMESTAMPTZ1", " <= TIMESTAMP '" + INITIAL_TS.format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.atZone(tz.toZoneId()).toInstant()) + Arguments.of("TIMESTAMPTZ1", " > TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.plusSeconds(8).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ.plusSeconds(9)), + Arguments.of("TIMESTAMPTZ1", " >= TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.plusSeconds(9).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ.plusSeconds(9)), + Arguments.of("TIMESTAMPTZ1", " < TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.plusSeconds(1).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ), + Arguments.of("TIMESTAMPTZ1", " <= TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ) ); } @@ -278,10 +279,10 @@ Arguments.of("TIMESTAMP2", "TIMESTAMP '" + INITIAL_TS.plusSeconds(5).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS.plusSeconds(5)), - Arguments.of("TIMESTAMPTZ1", "TIMESTAMP '" + INITIAL_TS.plusSeconds(5).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.plusSeconds(5).atZone(tz.toZoneId()).toInstant()), - Arguments.of("TIMESTAMPTZ2", "TIMESTAMP '" + INITIAL_TS.plusSeconds(5).format(SQL_CONFORMANT_DATETIME_FORMATTER) - + "'", INITIAL_TS.plusSeconds(5).atZone(tz.toZoneId()).toInstant()) + Arguments.of("TIMESTAMPTZ1", "TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.plusSeconds(5).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ.plusSeconds(5)), + Arguments.of("TIMESTAMPTZ2", "TIMESTAMP WITH LOCAL TIME ZONE '" + + INITIAL_TS.plusSeconds(5).format(SQL_CONFORMANT_DATETIME_FORMATTER) + "'", INITIAL_TS_LTZ.plusSeconds(5)) ); } @@ -297,9 +298,11 @@ break; case "TIMESTAMP1": case "TIMESTAMP2": + fill(table, INITIAL_TS::plusSeconds); + break; case "TIMESTAMPTZ1": case "TIMESTAMPTZ2": - fill(table, INITIAL_TS::plusSeconds); + fill(table, INITIAL_TS_LTZ::plusSeconds); break; default: throw new IllegalArgumentException("Undefined table: " + table); ``` WDYT? :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-unsubscr...@ignite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
