Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DRAFT] Defines spi.datetime classes #1656

Closed
wants to merge 9 commits into from
Closed

[DRAFT] Defines spi.datetime classes #1656

wants to merge 9 commits into from

Conversation

rchowell
Copy link
Contributor

Description

This PR works on cleaning up and finalizing the Datetime classes in accordance with the SQL specification with some minor changes. It also introduces the INTERVAL type which may be split into INTERVALYM and INTERVALDS and can be easily converted to a packed byte[] representation. This is a draft until the existing ones are replaced.

See #1654

Other Information

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.

@rchowell rchowell changed the title Backup the spi.datetime package [V1] Cleanup datetime and interval values Nov 25, 2024
@rchowell rchowell changed the title [V1] Cleanup datetime and interval values [V1] Defines spi.datetime classes Nov 26, 2024
@rchowell rchowell marked this pull request as ready for review November 26, 2024 17:39
Copy link

github-actions bot commented Nov 26, 2024

CROSS-ENGINE-REPORT ❌

BASE (LEGACY-V0.14.8) TARGET (EVAL-11B3DA9) +/-
% Passing 89.67% 10.35% -79.32% ⭕
Passing 5287 610 -4677 ⭕
Failing 609 5041 4432 ⭕
Ignored 0 245 245 🔶
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: v0.14.8
  • Base Engine: LEGACY
  • Target Commit: 11b3da9
  • Target Engine: EVAL

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
  • Passing in both: 231
  • Failing in both: 180
  • Ignored in both: 0
  • PASSING in BASE but now FAILING in TARGET: 2416
  • PASSING in BASE but now IGNORED in TARGET: 107
  • FAILING in BASE but now PASSING in TARGET: 17
  • IGNORED in BASE but now PASSING in TARGET: 0

Now FAILING Tests ❌

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

Now IGNORED Tests ❌

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

Now Passing Tests

17 test(s) were previously failing in BASE (LEGACY-V0.14.8) but now pass in TARGET (EVAL-11B3DA9). 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-48A9EE3) TARGET (EVAL-11B3DA9) +/-
% Passing 94.39% 10.35% -84.04% ⭕
Passing 5565 610 -4955 ⭕
Failing 50 5041 4991 ⭕
Ignored 281 245 -36 ✅
Total Tests 5896 5896 0 ✅

Testing Details

  • Base Commit: 48a9ee3
  • Base Engine: EVAL
  • Target Commit: 11b3da9
  • Target Engine: EVAL

Result Details

  • ❌ REGRESSION DETECTED. See Now Failing/Ignored Tests. ❌
  • Passing in both: 610
  • Failing in both: 48
  • Ignored in both: 243
  • PASSING in BASE but now FAILING in TARGET: 4955
  • 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

Now FAILING Tests ❌

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

Copy link
Member

@johnedquinn johnedquinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clean APIs. I haven't gone through all of it yet, but have some immediate questions.

* This class represents a PartiQL Interval value. SQL defines two "classes" of intervals: year-month and day-time.
* In Java these are represented by {@link Period} and {@link Duration} respectively.
*/
public interface Interval {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these variants, are you planning on adding getters for the fields? (years, months, etc)

Comment on lines 160 to 162
public static YM days(int days) {
return new YM(Period.ofDays(days));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If strictly SQL, days should return a DT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly SQL, just adding our own surface to Period/Duration. I don't like the split that much to be honest and I had started on our own interval which is when I realized I didn't want to re-invent datetime things. I do think our own interval could simplify things though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to flatten our values a bit more, these could immediately be Datum implementations. And Datum can expose the static methods. For example, Datum.intervalYearToMonth(int years, int months) returns the package private DatumIntervalYM implementation.

Same could go for Datum.intervalDayToHour(int days, int hours), Datum.intervalDayToMinute(int days, int hours, int minutes), etc pointing to the package private DatumIntervalDT.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, but I was hoping to separate our datetime impls from the Datum, since the Datum could just wrap them like it does now. But also there's no good reason for them to exist outside of Datum. Kind of like DatumDecimal wraps BigDecimal; we have the DatumDate/DatumTime today wrapping the existing Datetime stuff.

I'll work to internalize as much as possible behind Datum, ideally one day we actually do use stack values

@rchowell rchowell marked this pull request as draft November 26, 2024 22:23
@rchowell rchowell changed the title [V1] Defines spi.datetime classes [DRAFT] Defines spi.datetime classes Nov 27, 2024
Copy link
Member

@johnedquinn johnedquinn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! Left small comments. I'd like to see these APIs used in practice, so once the build and conformance tests pass, I can approve.

* will throw this exception upon invocation.
* @throws NullPointerException if this instance also returns true on {@link #isNull()}; callers should check that
* {@link #isNull()} returns false before attempting to invoke this method.
* @return a {@link LocalDate} for DATE, TIMESTAMP, and TIMESTAMPZ types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking -- Given TIMESTAMP/Z has getLocalDateTime, this may be redundant. Same goes for getLocalTime.

Datum.integer(v.minute)
}

//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to delete these?

@rchowell
Copy link
Contributor Author

This looks good to me! Left small comments. I'd like to see these APIs used in practice, so once the build and conformance tests pass, I can approve.

Sounds good, I didn't want to sink a bunch of time into updating function impls until the APIs were finalized.

@rchowell
Copy link
Contributor Author

Reduced scope and pushed as #1691

@rchowell rchowell closed this Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants