Add time-related types to the storage and transactional interfaces. #2437

Torch3333 · 2024-12-23T06:23:29Z

Description

This implements the CRUD interface for the DATE, TIME, TIMESTAMP, and TIMESTAMPTZ types.

The limitations are:

On Oracle: ScalarDB TIMESTAMPTZ cannot be used as a partition or clustering key because its storage type TIMESTAMP WITH TIME ZONE cannot be a primary key. Using it as a secondary index is possible.
On Cassandra, the TIMESTAMP type is currently not supported but can be in a future ScalarDB version

Remaining work:

Add integration tests for the two-phase commit interface
Support the importing table feature

Related issues and/or PRs

Add time-related types to the Admin and Schema loader #2301

Changes made

Added support to CRUD operation of the storage and transactional interface for the DateColumn, TimeColumn, TimestampColumn and TimestampTZColumn

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

Sorry for the huge number of changes, most of them concern constructor or builder method but I commented on the area that require more careful review.

This PR will be merged into the feature branch add_time_related_types

Release notes

N/A

Torch3333 · 2024-12-23T07:04:43Z

...tion-test/java/com/scalar/db/storage/jdbc/JdbcDatabaseCrossPartitionScanIntegrationTest.java

@@ -45,4 +50,25 @@ protected boolean isParallelDdlSupported() {
    }
    return super.isParallelDdlSupported();
  }
+
+  @Override
+  protected Stream<Arguments> provideColumnsForCNFConditionsTest() {


Modified the test DistributedStorageCrossPartitionScanIntegrationTestBase.scan_WithConjunctiveNormalFormConditionsShouldReturnProperResult for Oracle, SQLServer, and SQLite to reduce the size of the condition

Torch3333 · 2024-12-23T07:12:45Z

core/src/main/java/com/scalar/db/common/AbstractResult.java

-                    getColumns().entrySet().stream()
-                        .collect(
-                            Collectors.toMap(
-                                Entry::getKey, e -> ScalarDbUtils.toValue(e.getValue())))));


Since we do not add the implementation of the Value<T> type for time-related types because it is deprecated.
I removed any code from src/main that use Value<?> objects. Some unit and integration tests still use it extensively but that will need to be taken care of later on.

Torch3333 · 2024-12-23T07:14:55Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

@@ -690,6 +690,42 @@ public enum CoreError implements ScalarDbError {
      ""),
  DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT(
      Category.USER_ERROR, "0151", "Method null argument not allowed", "", ""),
+  OUT_OF_RANGE_COLUMN_VALUE_FOR_DATE(


Add some error codes.

@josh-wong Could you please take a look at these error messages?

Torch3333 · 2024-12-23T07:16:48Z

core/src/main/java/com/scalar/db/io/TimeColumn.java

+ * calendar system, such as 16:15:30, and can be expressed with microsecond precision.
+ */
+@Immutable
+public class TimeColumn implements Column<LocalTime> {


Add the TimeColumn type. Its value is backed by a LocalTime object.

Torch3333 · 2024-12-23T07:17:08Z

core/src/main/java/com/scalar/db/io/DateColumn.java

+ * calendar system, such as 2007-12-03
+ */
+@Immutable
+public class DateColumn implements Column<LocalDate> {


Add the DateColumn type. Its value is backed by the LocalDate object.

Torch3333 · 2024-12-23T07:40:27Z

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineOracle.java

+    return LocalDateTime.of(
+        config.getOracleTimeColumnDefaultDateComponent(), column.getTimeValue());


Encode any TIME value in Oracle with the default date component set in the configuration.

Torch3333 · 2024-12-23T08:56:27Z

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java

@@ -131,4 +143,56 @@ default String getPattern(LikeExpression likeExpression) {
  default @Nullable String getSchemaName(String namespace) {
    return namespace;
  }
+
+  default Object encodeDate(DateColumn column) {


The encoding of time-related types for JDBC is not straightforward as some drivers have specificities regarding time-zone handling or the use of dates before the transition from the Julian to the Gregorian Calendar on October 15th, 1582.

Torch3333 · 2024-12-23T08:59:18Z

...n-test/src/main/java/com/scalar/db/api/DistributedStorageColumnValueIntegrationTestBase.java

+  }
+
+  @Test
+  public void


Added two integration tests put_WithProblematicDateBecauseOfJulianToGregorianCalendarTransition_ShouldPutCorrectly and put_forTimeRelatedTypesWithVariousJvmTimezone_ShouldPutCorrectly to cover for some corner cases.
See the comment in the test for more details.

Torch3333 · 2024-12-23T09:10:40Z

build.gradle

@@ -31,7 +31,7 @@ subprojects {
        commonsDbcp2Version = '2.13.0'
        mysqlDriverVersion = '8.4.0'
        postgresqlDriverVersion = '42.7.4'
-        oracleDriverVersion = '21.16.0.0'
+        oracleDriverVersion = '23.6.0.24.10'


Upgrade the Oracle driver to the latest version to facilitate the handling of dates before October 15th, 1582.

Torch3333 · 2024-12-23T10:04:57Z

integration-test/src/main/java/com/scalar/db/api/DistributedTransactionIntegrationTestBase.java

@@ -93,10 +111,32 @@ protected String getNamespaceBaseName() {
  }

  private void createTables() throws ExecutionException {
+    TableMetadata.Builder tableMetadata =


Up until now DistributedTransactionIntegrationTestBase was performing tests on a table composed of columns of INT type only.
I updated several tests that targeted the most generic use case for each operation to target all data types.

komamitsu · 2024-12-24T04:58:59Z

...tion-test/java/com/scalar/db/storage/jdbc/JdbcDatabaseSinglePartitionKeyIntegrationTest.java

+    return super.getPartitionKeyTypes().stream()
+        .filter(
+            type -> {
+              if (JdbcTestUtils.isOracle(rdbEngine) && type == DataType.TIMESTAMPTZ) {


I think this logic is a bit complicated and shouldn't be duplicated. How about moving this to a util class to reuse it?

You are right, I created a util method JdbcTestUtils.filterDataTypes(...). Thank you.
adcbfde

komamitsu · 2024-12-24T06:42:46Z

core/src/main/java/com/scalar/db/io/DateColumn.java

+
+  @Override
+  protected final void finalize() throws Throwable {
+    // TODO delete this method once https://github.com/scalar-labs/scalardb/pull/2421 is merge


#2421 is merged 👍

Thanks, I will do that in a separate PR because rebasing the feature branch add_time_related_types now would mess up this PR I think 🤔

core/src/main/java/com/scalar/db/storage/jdbc/JdbcUtils.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineMysql.java

komamitsu · 2024-12-24T11:55:29Z

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlServer.java

+  @Override
+  public DateTimeOffset encodeTimestampTZ(TimestampTZColumn column) {
+    assert column.getTimestampTZValue() != null;
+    // When using SQLServer DATETIMEOFFSET data type, we should use the SQLServer JDBC driver's


Does this mean using Instant or LocalDateTime fails with SQLServer?

Yes, for encoding data, only using the SQLServer driver microsoft.sql.DateTimeOffset type worked for the range of data I tested.
Decoding through a Java java.time.OffsetDateTime object worked, if my memory is correct, microsoft.sql.DateTimeOffset was ok too.

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java

komamitsu

LGTM! 👍

Great work!

brfrn169

Overall LGTM. Left several comments. Please take a look when you have time!

...ain/java/com/scalar/db/api/DistributedStorageSingleClusteringKeyScanIntegrationTestBase.java

brfrn169 · 2024-12-25T14:19:53Z

core/src/main/java/com/scalar/db/common/error/CoreError.java

@@ -690,6 +690,42 @@ public enum CoreError implements ScalarDbError {
      ""),
  DATA_LOADER_ERROR_METHOD_NULL_ARGUMENT(
      Category.USER_ERROR, "0151", "Method null argument not allowed", "", ""),
+  OUT_OF_RANGE_COLUMN_VALUE_FOR_DATE(


@josh-wong Could you please take a look at these error messages?

core/src/main/java/com/scalar/db/io/Key.java

brfrn169 · 2024-12-25T14:25:23Z

core/src/main/java/com/scalar/db/storage/TimeRelatedColumnEncodingUtils.java

+/**
+ * This class provides utility methods for encoding and decoding time related column value for
+ * DynamoDB, CosmosDB and SQLite
+ */
+public final class TimeRelatedColumnEncodingUtils {


Could you please move this class under com.scalar.db.util?

brfrn169

LGTM! Thank you!

josh-wong

I've added some comments and suggestions. PTAL!

core/src/main/java/com/scalar/db/common/error/CoreError.java

Co-authored-by: Josh Wong <[email protected]>

…tion on time-related columns

…scalardb into add_time_related_types_CRUD # Conflicts: # core/src/main/java/com/scalar/db/common/error/CoreError.java

josh-wong

Sorry, there was one minor thing I noticed about the wording, so I've added some suggestions. PTAL!

core/src/main/java/com/scalar/db/common/error/CoreError.java

feeblefakie

Overall, looking good to me. Thank you!
I left a few comments. PTAL!

feeblefakie · 2025-01-03T11:51:01Z

core/src/main/java/com/scalar/db/io/TimestampTZColumn.java

+ * precision.
+ */
+@Immutable
+public class TimestampTZColumn implements Column<Instant> {


Suggested change

public class TimestampTZColumn implements Column<Instant> {

public class TimestampTzColumn implements Column<Instant> {

Probably? (based on the naming convention in the style guide)

I also hesitated between TimestampTZ and TimestampTz but chose the first one for the following reasons:

My understanding of the Google Java style guide differs from yours. Since time zone is written in two words, it recommends capitalizing the T and the Z.

Compared to TimestampColumn, TimestampTZColumn stands out more than TimestampTzColumn. So, it is easier to spot the difference between the two types when reading code and writing code using auto-completion.

I could only find two Java classes with a similar naming and both used the capital letter version. For example in Apache Hive and Spark

What do you think?

feeblefakie · 2025-01-03T12:36:43Z

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineStrategy.java

 import javax.annotation.Nullable;

 /**
 * An interface to hide the difference between underlying JDBC SQL engines in SQL dialects, error
 * codes, and so on. It's NOT responsible for actually connecting to underlying engines.
 */
-public interface RdbEngineStrategy {
+public interface RdbEngineStrategy<T_DATE, T_TIME, T_TIMESTAMP, T_TIMESTAMPTZ> {


I don't have a strong opinion, but having four time-related generics in this interface might be too much since this handles many other data types.
How about pushing the time-related type things to other interface and classes?
For example, how about creating a time-specific strategy that converts the default type to a DB-specific type?
For example, we can create RdbEngineTimeTypeStrategy, and it converts time-related types as necessary as follows.

preparedStatement.setObject(index++, rdbEngineTimeType.convert(rdbEngine.encodeTimestampTZ(column)));

It sounds good, thank you.
I reverted an earlier commit based on @komamitsu suggestion in 0eee567 then added your suggestion in
fe3be8d

This reverts commit be71af5.

Co-authored-by: Josh Wong <[email protected]>

…scalardb into add_time_related_types_CRUD

josh-wong

LGTM! Thank you!🙇🏻‍♂️

Does this PR need to have labels and projects selected? Or is it not necessary since it'll be merged into an existing branch where other work is being done?

Torch3333 · 2025-01-07T00:42:46Z

@josh-wong

LGTM! Thank you!🙇🏻‍♂️

Does this PR need to have labels and projects selected? Or is it not necessary since it'll be merged into an existing branch where other work is being done?

Yes, the current PR is merged into a temporary feature branch, so we don't add labels or projects. I will add tags and labels to the PR to merge the feature branch into master.

Torch3333 force-pushed the add_time_related_types_CRUD branch from 1360d38 to be37237 Compare December 23, 2024 06:55

Torch3333 self-assigned this Dec 23, 2024

Add CRUD for time-related types

57cbb31

Torch3333 force-pushed the add_time_related_types_CRUD branch from 0e70eb5 to 57cbb31 Compare December 23, 2024 09:08

Torch3333 changed the title ~~Add time related types crud~~ Add time-related types to the storage and transactional interfaces. Dec 23, 2024

Torch3333 commented Dec 23, 2024

View reviewed changes

Torch3333 marked this pull request as ready for review December 23, 2024 10:16

Torch3333 requested review from komamitsu, brfrn169 and feeblefakie December 23, 2024 10:16

komamitsu reviewed Dec 24, 2024

View reviewed changes

Torch3333 added 3 commits December 25, 2024 15:28

Factorise data type exclusion for JDBC

adcbfde

Type safety on RdbEngine.encodeX() methods

be71af5

Don't check if property exist before adding

bc66fee

Torch3333 requested a review from komamitsu December 25, 2024 06:32

komamitsu approved these changes Dec 25, 2024

View reviewed changes

Fix comment in TimeRelatedColumnEncodingUtils.java [skip ci]

dce86f7

brfrn169 requested a review from josh-wong December 25, 2024 14:19

brfrn169 reviewed Dec 25, 2024

View reviewed changes

Torch3333 added 3 commits December 26, 2024 09:41

Remove deprecated Key constructor for time-related types

0b774d2

Move TimeRelatedColumnEncodingUtils to util package

edb1814

Fix typo in error message

8bc05bc

Torch3333 requested a review from brfrn169 December 26, 2024 00:51

brfrn169 approved these changes Dec 26, 2024

View reviewed changes

josh-wong requested changes Dec 26, 2024

View reviewed changes

Torch3333 and others added 4 commits December 27, 2024 13:15

Capitalize data types

8a3f914

Apply Josh suggestions [skip ci]

1b3b732

Co-authored-by: Josh Wong <[email protected]>

Update Admin integration test about index creation to test data inser…

a02b284

…tion on time-related columns

Merge branch 'add_time_related_types_CRUD' of github.com:scalar-labs/…

e1c11ed

…scalardb into add_time_related_types_CRUD # Conflicts: # core/src/main/java/com/scalar/db/common/error/CoreError.java

Torch3333 requested a review from josh-wong December 27, 2024 04:40

Update typo on type name in CoreError.java [skip ci]

c739f3d

josh-wong requested changes Dec 27, 2024

View reviewed changes

core/src/main/java/com/scalar/db/common/error/CoreError.java Outdated Show resolved Hide resolved

core/src/main/java/com/scalar/db/common/error/CoreError.java Outdated Show resolved Hide resolved

core/src/main/java/com/scalar/db/common/error/CoreError.java Outdated Show resolved Hide resolved

feeblefakie reviewed Jan 3, 2025

View reviewed changes

Torch3333 and others added 4 commits January 6, 2025 12:32

Revert "Type safety on RdbEngine.encodeX() methods"

0eee567

This reverts commit be71af5.

Apply suggestions from Josh [skip ci]

05382e0

Co-authored-by: Josh Wong <[email protected]>

Refactor RdbEngineStrategy

fe3be8d

Merge branch 'add_time_related_types_CRUD' of github.com:scalar-labs/…

abf304a

…scalardb into add_time_related_types_CRUD

Torch3333 requested review from feeblefakie and josh-wong January 6, 2025 05:15

josh-wong approved these changes Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add time-related types to the storage and transactional interfaces. #2437

Add time-related types to the storage and transactional interfaces. #2437

Torch3333 commented Dec 23, 2024 •

edited

Loading

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

brfrn169 Dec 25, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024

Torch3333 Dec 23, 2024 •

edited

Loading

komamitsu Dec 24, 2024

Torch3333 Dec 25, 2024 •

edited

Loading

komamitsu Dec 24, 2024

Torch3333 Dec 25, 2024 •

edited

Loading

komamitsu Dec 24, 2024

Torch3333 Dec 25, 2024

komamitsu left a comment

brfrn169 left a comment

brfrn169 Dec 25, 2024

brfrn169 Dec 25, 2024

brfrn169 left a comment

josh-wong left a comment

josh-wong left a comment

feeblefakie left a comment

feeblefakie Jan 3, 2025

Torch3333 Jan 6, 2025 •

edited

Loading

feeblefakie Jan 3, 2025

Torch3333 Jan 6, 2025 •

edited

Loading

josh-wong left a comment •

edited

Loading

Torch3333 commented Jan 7, 2025

		return LocalDateTime.of(
		config.getOracleTimeColumnDefaultDateComponent(), column.getTimeValue());

	public class TimestampTZColumn implements Column<Instant> {
	public class TimestampTzColumn implements Column<Instant> {

Add time-related types to the storage and transactional interfaces. #2437

Are you sure you want to change the base?

Add time-related types to the storage and transactional interfaces. #2437

Conversation

Torch3333 commented Dec 23, 2024 • edited Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 Dec 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 Dec 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brfrn169 left a comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

josh-wong left a comment

Choose a reason for hiding this comment

feeblefakie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Torch3333 Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

josh-wong left a comment • edited Loading

Choose a reason for hiding this comment

Torch3333 commented Jan 7, 2025

Torch3333 commented Dec 23, 2024 •

edited

Loading

Torch3333 Dec 23, 2024 •

edited

Loading

Torch3333 Dec 25, 2024 •

edited

Loading

Torch3333 Dec 25, 2024 •

edited

Loading

Torch3333 Jan 6, 2025 •

edited

Loading

Torch3333 Jan 6, 2025 •

edited

Loading

josh-wong left a comment •

edited

Loading