Updates Datum to use java.time for a sane datetime #1691

RCHowell · 2024-12-24T21:13:12Z

Description

This PR updates Datum to use java.time.

See #1654

Other Information

Updated Unreleased Section in CHANGELOG: NO
Any backward-incompatible changes? NO
Any new external dependencies? NO
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? YES

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

< If YES, which ones and why? >
< In addition, please also mention any other alternatives you've considered and the reason they've been discarded >
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]

License Information

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

github-actions · 2024-12-24T21:19:15Z

CROSS-ENGINE-REPORT ❌

	BASE (LEGACY-V0.14.8)	TARGET (EVAL-BF3FE0F)	+/-
% Passing	89.67%	94.32%	4.65% ✅
Passing	5287	5561	274 ✅
Failing	609	54	-555 ✅
Ignored	0	281	281 🔶
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: v0.14.8
Base Engine: LEGACY
Target Commit: bf3fe0f
Target Engine: EVAL

Result Details

❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
Passing in both: 2641
Failing in both: 17
Ignored in both: 0
PASSING in BASE but now FAILING in TARGET: 5
PASSING in BASE but now IGNORED in TARGET: 108
FAILING in BASE but now PASSING in TARGET: 180
IGNORED in BASE but now PASSING in TARGET: 0

Now FAILING Tests ❌

The following 5 test(s) were previously PASSING in BASE but are now FAILING in TARGET:

Click here to see

undefinedUnqualifiedVariableWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE
undefinedUnqualifiedVariableIsNullExprWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE
undefinedUnqualifiedVariableIsMissingExprWithUndefinedVariableBehaviorMissing, compileOption: PERMISSIVE
inPredicateWithTableConstructor, compileOption: PERMISSIVE
notInPredicateWithTableConstructor, compileOption: PERMISSIVE

Now IGNORED Tests ❌

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

Now Passing Tests

180 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-BF3FE0F). Before merging, confirm they are intended to pass.

The complete list can be found in GitHub CI summary, either from Step Summary or in the Artifact.

CROSS-COMMIT-REPORT ✅

	BASE (EVAL-EA779EE)	TARGET (EVAL-BF3FE0F)	+/-
% Passing	94.32%	94.32%	0.00% ✅
Passing	5561	5561	0 ✅
Failing	54	54	0 ✅
Ignored	281	281	0 ✅
Total Tests	5896	5896	0 ✅

Testing Details

Base Commit: ea779ee
Base Engine: EVAL
Target Commit: bf3fe0f
Target Engine: EVAL

Result Details

Passing in both: 5561
Failing in both: 54
Ignored in both: 281
PASSING in BASE but now FAILING in TARGET: 0
PASSING in BASE but now IGNORED in TARGET: 0
FAILING in BASE but now PASSING in TARGET: 0
IGNORED in BASE but now PASSING in TARGET: 0

alancai98 · 2024-12-26T18:23:50Z

partiql-spi/src/main/java/org/partiql/spi/value/DatumNull.java


 import java.math.BigDecimal;
 import java.math.BigInteger;
+import java.time.*;


nit: style-wise can use explicit imports

alancai98 · 2024-12-26T18:28:56Z

partiql-spi/src/main/java/org/partiql/spi/value/DatumTimestamp.java

 import org.jetbrains.annotations.NotNull;
 import org.partiql.types.PType;
-import org.partiql.value.datetime.Timestamp;
+
+import java.time.*;


nit: similarly here, can use explicit imports

alancai98 · 2024-12-26T18:29:30Z

partiql-spi/src/main/java/org/partiql/spi/value/DatumTimestampz.java

+import org.jetbrains.annotations.NotNull;
+import org.partiql.types.PType;
+
+import java.time.*;


nit: explicit imports

alancai98 · 2024-12-26T18:30:52Z

partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnBetween.kt

-    val upper = args[2].time
+    val value = args[0].localTime
+    val lower = args[1].localTime
+    val upper = args[2].localTime


Seems like we're missing function definitions for timez and timestampz? Will those be added at some point? Similarly for the other builtin functions?

alancai98 · 2024-12-26T18:37:47Z

partiql-spi/src/main/kotlin/org/partiql/spi/function/builtins/FnUtcnow.kt

-    val now = TimestampWithTimeZone.nowZ()
-    Datum.timestamp(now)
+    val now = LocalDateTime.now()
+    Datum.timestamp(now, 6)


Should this be a timestamp or timestampz?

alancai98 · 2024-12-26T18:41:38Z

test/partiql-tests-runner/src/test/kotlin/org/partiql/runner/io/DatumIonReader.kt

+                        // calculate second/nanos
+                        val ds = second.bigDecimal
+                        val seconds: Int = ds.toInt()
+                        val nanoOfSecond: Int = ds.remainder(BigDecimal.ONE).movePointRight(9).toInt()


nit: this is used in a few other places. perhaps could be some top-level variable/const?

alancai98 · 2024-12-26T18:41:50Z

...s/src/test/kotlin/org/partiql/lang/randomized/syntax/PartiQLParserDateTimeRandomizedTests.kt

-
-    @Test
-    @Disabled("The planner does not return the v1 plans right now. See assertExpression.") // TODO
-    fun testRandomDates() {


Why is this test getting removed?

alancai98 · 2024-12-26T18:45:36Z

partiql-spi/src/main/java/org/partiql/spi/value/Datum.java

@@ -736,18 +727,28 @@ static Datum blob(@NotNull byte[] value, int length) {
    // DATE/TIME

    @NotNull
-    static Datum date(@NotNull Date value) {
+    static Datum date(@NotNull LocalDate value) {


Perhaps I'm missing some context for the changes here. I had a couple questions

Why are we opting to use the Java-defined datetime classes rather than the currently-defined datetime classes/interfaces in SPI? I'm not opposed but just would like some more context here.

Will those datetime classes/interfaced defined in SPI be deleted at some point?

RCHowell added 5 commits December 24, 2024 09:33

Updates datum datetime to use java.time

f330e3c

Updates IonVariant for datetime java.time

8c2afda

Assemble partiql-spi and apiDump

392bfef

Fix partiql-spi build

a888160

Updates CAST and connversions

b337934

RCHowell mentioned this pull request Dec 24, 2024

[DRAFT] Defines spi.datetime classes #1656

Draft

RCHowell changed the title ~~Updates Datum to use java.time for a sane datetime.~~ Updates Datum to use java.time for a sane datetime Dec 24, 2024

RCHowell added 2 commits December 24, 2024 13:36

Adds old datetime to datum conversions

d07e02f

Adds missing CASTS for time/timez

3c2431b

alancai98 reviewed Dec 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates Datum to use java.time for a sane datetime #1691

Updates Datum to use java.time for a sane datetime #1691

RCHowell commented Dec 24, 2024

github-actions bot commented Dec 24, 2024 •

edited

Loading

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

alancai98 Dec 26, 2024

Updates Datum to use java.time for a sane datetime #1691

Are you sure you want to change the base?

Updates Datum to use java.time for a sane datetime #1691

Conversation

RCHowell commented Dec 24, 2024

Description

Other Information

License Information

License Information

github-actions bot commented Dec 24, 2024 • edited Loading

CROSS-ENGINE-REPORT ❌

Testing Details

Result Details

Now FAILING Tests ❌

Now IGNORED Tests ❌

Now Passing Tests

CROSS-COMMIT-REPORT ✅

Testing Details

Result Details

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

github-actions bot commented Dec 24, 2024 •

edited

Loading