Timestamp Data Model #1121

yliuuuu · 2023-06-13T00:04:02Z

Relevant Issues

PartiQL Doc: issue 41

Description

This PR is a POC on Timestamp Type.
This PR covers the Timestamp Data Model, and enable timestamp constructor in Parser.
This PR does not cover any aspect of evaluation yet, as covering those will create too big a diff for reviewer.

Other Information

Updated Unreleased Section in CHANGELOG: [YES/NO]
- Yes.
Any backward-incompatible changes? [YES/NO]
- Not yet, but will be once we gets to the compiler/evaluator.
Any new external dependencies? [YES/NO]
- 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.

CHANGELOG.md

github-actions · 2023-06-22T21:39:53Z

Conformance comparison report

	Base (`2283db5`)	`6ea7b97`	+/-
% Passing	92.40%	92.40%	0.00%
✅ Passing	5376	5376	0
❌ Failing	442	442	0
🔶 Ignored	0	0	0
Total Tests	5818	5818	0

Number passing in both: 5376

Number failing in both: 442

Number passing in Base (2283db5) but now fail: 0

Number failing in Base (2283db5) but now pass: 0

codecov-commenter · 2023-06-22T23:19:57Z

Codecov Report

Patch coverage: 85.71% and project coverage change: +0.10 🎉

Comparison is base (2283db5) 73.19% compared to head (f3b48e7) 73.30%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1121      +/-   ##
============================================
+ Coverage     73.19%   73.30%   +0.10%     
- Complexity     2376     2400      +24     
============================================
  Files           237      238       +1     
  Lines         17261    17444     +183     
  Branches       3133     3158      +25     
============================================
+ Hits          12635    12787     +152     
- Misses         3653     3674      +21     
- Partials        973      983      +10

Flag	Coverage Δ
CLI	`14.28% <ø> (ø)`
EXAMPLES	`80.28% <ø> (ø)`
LANG	`79.15% <85.71%> (+0.04%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...kotlin/org/partiql/lang/eval/EvaluatingCompiler.kt	`80.70% <0.00%> (-0.14%)`	⬇️
...iql/lang/eval/physical/PhysicalPlanCompilerImpl.kt	`83.86% <0.00%> (-0.26%)`	⬇️
.../eval/visitors/SubqueryCoercionVisitorTransform.kt	`96.70% <0.00%> (-2.19%)`	⬇️
...n/org/partiql/lang/prettyprint/ASTPrettyPrinter.kt	`77.91% <0.00%> (-0.15%)`	⬇️
...org/partiql/lang/prettyprint/QueryPrettyPrinter.kt	`78.37% <0.00%> (-0.29%)`	⬇️
...artiql/lang/types/PartiqlPhysicalTypeExtensions.kt	`90.00% <0.00%> (-2.76%)`	⬇️
...tlin/org/partiql/lang/syntax/util/DateTimeUtils.kt	`79.54% <79.54%> (ø)`
.../org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt	`89.47% <88.32%> (+0.16%)`	⬆️
...c/main/kotlin/org/partiql/lang/errors/ErrorCode.kt	`85.21% <100.00%> (+0.24%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

johnedquinn

This is an awesome piece of work. Left some comments -- the only two files I have yet to go through are TimestampTests and PartiQLParserDateTimeTests.

johnedquinn · 2023-06-28T18:07:48Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/Time.kt

+    /**
+     * Counting the time escaped from midnight 00:00:00 in seconds ( fraction included)
+     */
+    val elapsedSecond: BigDecimal by lazy {


And, roundToPrecision can use this value.

This may not need to be internal as this is a commonly exposed property in java's time package.
Maybe keep it as is and wait for the evaluation PR to confirm?

partiql-types/src/main/kotlin/org/partiql/value/datetime/Time.kt

partiql-types/src/main/kotlin/org/partiql/value/datetime/TimeZone.kt

partiql-types/src/main/kotlin/org/partiql/value/datetime/Timestamp.kt

partiql-ast/src/main/pig/partiql.ion

partiql-types/src/main/kotlin/org/partiql/value/datetime/Timestamp.kt

partiql-lang/src/main/kotlin/org/partiql/lang/syntax/util/DateTimeUtils.kt

partiql-lang/src/main/kotlin/org/partiql/lang/syntax/impl/PartiQLPigVisitor.kt

partiql-types/src/test/kotlin/org/partiql/datetime/TimestampTest.kt

yliuuuu

Address feedback

yliuuuu · 2023-07-03T16:57:28Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/Time.kt

+    /**
+     * Counting the time escaped from midnight 00:00:00 in seconds ( fraction included)
+     */
+    val elapsedSecond: BigDecimal by lazy {


This may not need to be internal as this is a commonly exposed property in java's time package.
Maybe keep it as is and wait for the evaluation PR to confirm?

yliuuuu · 2023-07-03T17:00:17Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/Timestamp.kt

+    }
+
+    val epochMillis: BigDecimal by lazy {
+        epochSecond.movePointRight(3)


This should be safe.

partiql-types/src/main/kotlin/org/partiql/value/datetime/Timestamp.kt

RCHowell

Overall very impressive. This is a ton of work. Just a few comments as I took a cursory review. I will look more into this tomorrow. Thank you.

RCHowell · 2023-07-10T23:18:12Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/DateTime.kt

+    public override val year: Int?
+        get() = null
+
+    /**
+     * Month field for [Time] is always null
+     */
+    public override val month: Int?
+        get() = null
+
+    /**
+     * Day field for [Time] is always null
+     */
+    public override val day: Int?
+        get() = null


Why getters? Can you just assign null?

Property initializers are not allowed in interfaces.

RCHowell · 2023-07-10T23:20:07Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/DateTime.kt

+public abstract class DateImpl : Date, Comparable<Date> {
+    public final override fun equals(other: Any?): Boolean {
+        if (this === other) return true
+        if (other?.javaClass != javaClass) return false
+        other as DateImpl
+        if (this.year != other.year) return false
+        if (this.month != other.month) return false
+        if (this.day != other.day) return false
+        return true
+    }
+
+    public final override fun hashCode(): Int =
+        year.hashCode() + month.hashCode() + day.hashCode()
+
+    public final override fun toString(): String =
+        "${this.javaClass.simpleName}(year=$year, month=$month, day=$day)"
+}


Why not make Date the abstract class with these default implementations?

The original thought was to accommodate the possibility of having to implement different date system. Maybe I am overcomplicating this stuff and we would never need this...

johnedquinn

Wow -- this is quite a lot of work. Great job. As for the creation of low/high precision datetimes, I didn't find anything wrong. I had some questions offline, but it all looked good.

I had one small nit about a function name and a test method, but beyond that, it LGTM.

johnedquinn · 2023-07-11T16:05:32Z

partiql-types/src/main/kotlin/org/partiql/value/datetime/DateTime.kt

+     * Returns a [Date] value with the specified number of months added.
+     * [years] can be negative.
+     */
+    public fun plusYear(years: Long): Date


Nit -- plusYears

Also, specified number of years added in the KDoc.

johnedquinn · 2023-07-11T21:13:57Z

partiql-types/src/test/kotlin/org/partiql/datetime/TimestampTest.kt

+            // negative epoch
+            // May be Ion has a bug?
+            // Expected epoch in Millis : -23225270400000,
+            // Actual epoch in Millis   : -23225875200000
+            // SuccessCaseWithNoTimeZone(
+            //    1234, 1, 1, 0,0, BigDecimal.valueOf(0,3), 0,
+            //    Timestamp.of(
+            //        DateTimeValue.date(1234, 1, 1),
+            //        DateTimeValue.time(0,0, BigDecimal.valueOf(0,0), )
+            // ),
+            // ),


While the matching SuccessCaseWithUnknownTimeZone fails -- would this one also fail? This specific test case shouldn't be comparing Ion values if I'm reading this correctly.

yliuuuu added 5 commits June 22, 2023 12:47

timestamp data model

86e17b1

parser tests

7d9f5ed

clean up

7b111af

changelog

e231d31

typo

8e1ae21

yliuuuu force-pushed the TimestampType branch from bb48545 to 8e1ae21 Compare June 22, 2023 21:33

yliuuuu marked this pull request as ready for review June 22, 2023 21:36

yliuuuu changed the title ~~PoC on timestamp~~ Timestamp Data Model Jun 22, 2023

yliuuuu commented Jun 22, 2023

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

yliuuuu added 5 commits June 22, 2023 14:52

clean up Exception

e922d00

ktlint

c99c16f

changelog

2c3423c

add error test case

53acc03

submodule update

3b1b33b

yliuuuu requested review from johnedquinn and RCHowell June 23, 2023 00:01

yliuuuu added 2 commits June 27, 2023 16:08

porting from eval PR

8999aba

Merge branch 'main' into TimestampType

43253c3

johnedquinn requested changes Jun 28, 2023

View reviewed changes

yliuuuu added 2 commits July 3, 2023 09:52

address feedback

fbcf8e6

kdoc

10d7b12

yliuuuu requested a review from johnedquinn July 5, 2023 20:00

yliuuuu commented Jul 5, 2023

View reviewed changes

refactor data model

f3b48e7

RCHowell reviewed Jul 10, 2023

View reviewed changes

johnedquinn reviewed Jul 11, 2023

View reviewed changes

Address Feedback

82be805

yliuuuu requested a review from johnedquinn July 11, 2023 22:15

johnedquinn approved these changes Jul 11, 2023

View reviewed changes

yliuuuu merged commit 44e8a8d into main Jul 11, 2023
10 checks passed

yliuuuu deleted the TimestampType branch July 11, 2023 22:21

yliuuuu mentioned this pull request Jul 13, 2023

timestamp parsing partiql/partiql-tests#102

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Timestamp Data Model #1121

Timestamp Data Model #1121

yliuuuu commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023 •

edited

Loading

codecov-commenter commented Jun 22, 2023 •

edited

Loading

johnedquinn left a comment

johnedquinn Jun 28, 2023

johnedquinn Jun 28, 2023

yliuuuu Jul 3, 2023

yliuuuu left a comment

yliuuuu Jul 3, 2023

yliuuuu Jul 3, 2023

RCHowell left a comment

RCHowell Jul 10, 2023

yliuuuu Jul 10, 2023

RCHowell Jul 10, 2023

yliuuuu Jul 10, 2023

johnedquinn left a comment

johnedquinn Jul 11, 2023

johnedquinn Jul 11, 2023

johnedquinn Jul 11, 2023

Timestamp Data Model #1121

Timestamp Data Model #1121

Conversation

yliuuuu commented Jun 13, 2023 • edited Loading

Relevant Issues

Description

Other Information

License Information

github-actions bot commented Jun 22, 2023 • edited Loading

Conformance comparison report

codecov-commenter commented Jun 22, 2023 • edited Loading

Codecov Report

johnedquinn left a 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

yliuuuu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RCHowell left a 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

johnedquinn left a 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

yliuuuu commented Jun 13, 2023 •

edited

Loading

github-actions bot commented Jun 22, 2023 •

edited

Loading

codecov-commenter commented Jun 22, 2023 •

edited

Loading