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

Adds support for the interval type #1741

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

johnedquinn
Copy link
Member

@johnedquinn johnedquinn commented Feb 18, 2025

Relevant Issues

Overview

  • Added parsing/planning/compilation/execution support for the interval type(s).
  • Also adds multiple operations involving intervals.
  • Needed to write a new DatumTextWriter, since PartiQLValue doesn't support interval whatsoever.

SQL

If you are reviewing this PR, I highly recommend that you read:

  1. SQL:2023 Section 4.7, Datetimes and intervals
  2. SQL:2023 Section 4.7.3, Intervals
  3. SQL:2023 Section 4.7.4, Operations involving datetimes and intervals
  4. SQL:2023 Section 6.13 <cast specification>
    a. This gives insight into why we use INTERVAL_YM and INTERVAL_DT.
  5. SQL:1999 Section 4.12, Type conversions and mixing of data types
    a. This gives insight into the coercion of intervals

PType Design

This PR adds two new PTypes, INTERVAL_YM and INTERVAL_DT, to represent the year-month intervals and day-time intervals, respectively. We have taken this approach due to practical implementation reasons. The main reason is that all of the existing cast/coercion logic is done via the PType codes -- and, year-month intervals are mutually assignable and comparable to other year-month intervals. Same goes for day-time intervals.

We have added getIntervalCode(). This is mostly how PostgreSQL does it.

We have also added getFractionalPrecision(). Adding a new method to the interface does NOT slow down the implementation. We could have used getScale(), however, this might have led to confusion.

We have opted to use getPrecision() for getting the leading field precision. Though, if there is pushback, we can create a new API more similarly named to how it is called in the SQL Spec. Maybe getLeadingFieldPrecision(). It doesn't matter to me. I've documented how getPrecision() should be used when the type's code is INTERVAL_YM or INTERVAL_DT.

Datum Design

SQL describes intervals as a contiguous subset of the allowable interval types. For year-month intervals, it can be years or months (or a contiguous subset). For day-times, it can be days, hours, minutes, and seconds (or a contiguous subset). Therefore, year-month intervals may call getYears() and getMonths(). For day-time intervals, you may call getDays(), getHours(), getMinutes(), getSeconds(), and getNanos(). This way, we don't have Java's Duration or Period in our public API. We only deal with primitives, and if we really want to use some of the standard Java library's logic, we can under-the-hood convert them. By only dealing with primitives, we set up our APIs for future optimization without sacrificing any of Java's provided standard library logic. These are not mutually exclusive.

I only wish we took this approach for TIME, TIMEZ, TIMESTAMP, and TIMESTAMPZ. If we want to do this, I don't think it'd be too difficult.

Operations Added

This PR has added the following operations involving intervals:

  1. Datetime - Datetime = Interval
  2. Datetime + Interval = Datetime
  3. Datetime - Interval = Datetime
  4. Interval + Datetime = Datetime
  5. Interval + Interval = Interval
  6. Interval - Interval = Interval

For the above, there are many permutations of the datetimes and intervals for each of these operations (consider DATE, TIME, TIMEZ, TIMESTAMP, TIMESTAMPZ operating with INTERVAL YEAR/MONTH/YEAR_MONTH/DAY/etc)

Other operations that SQL specifies and are not implemented by this PR:

  • Interval * Numeric = Interval
  • Interval / Numeric = Interval
  • Numeric * Interval = Interval

If these are considered critical, I will add them to the PR. Or, if I find time before this is fully reviewed, I will add them.

License Information

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

@johnedquinn johnedquinn force-pushed the main-interval branch 6 times, most recently from cc667bf to fb00a50 Compare February 19, 2025 19:27
@@ -51,4 +55,89 @@ class OpArithmeticTest : PartiQLTyperTestBase() {

return super.testGenPType("arithmetic", tests, argsMap)
}

@TestFactory
fun plus(): Stream<DynamicContainer> {
Copy link
Member Author

@johnedquinn johnedquinn Feb 19, 2025

Choose a reason for hiding this comment

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

Self-review: I copied the arithmetic test from above to split out the plus/minus planner tests, since we can now add intervals to datetimes and vice-versa.

I would rather see end-to-end tests for this, but I digress.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.03%. Comparing base (4bf549b) to head (1222709).
Report is 28 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1741   +/-   ##
=========================================
  Coverage     80.03%   80.03%           
  Complexity       47       47           
=========================================
  Files            19       19           
  Lines           506      506           
  Branches         23       23           
=========================================
  Hits            405      405           
  Misses           88       88           
  Partials         13       13           
Flag Coverage Δ
EXAMPLES 80.03% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnedquinn johnedquinn force-pushed the main-interval branch 2 times, most recently from a0a2c8a to 0cfd212 Compare February 19, 2025 22:40
Adds several functions pertaining to INTERVAL

Adds coercions to/from INTERVAL
@johnedquinn johnedquinn marked this pull request as ready for review February 19, 2025 23:18
@johnedquinn johnedquinn changed the title [DRAFT] Adds support for the interval type Adds support for the interval type Feb 19, 2025
@johnedquinn johnedquinn requested a review from jpschorr February 20, 2025 21:56
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.

[V1] Complete datetime and interval types PartiQL support for INTERVAL type and literal value parsing
2 participants