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

implement PostgreSQL-compatible interval type #1444

Merged
merged 4 commits into from Jan 14, 2020
Merged

implement PostgreSQL-compatible interval type #1444

merged 4 commits into from Jan 14, 2020

Conversation

sploiselle
Copy link
Contributor

This PR refactors interval to improve PostgreSQL compatibility.

Biggest changes include:

  • Mixed intervals, (i.e. months and durations) which are required for Postgres support.
  • Supports both SQL standard and PostgreSQL-style interval strings (1-2 3 4:5:6:7 and 1 year 2 months... respectively).
  • PostgreSQL consistent behavior for DateTimeField ranges (INTERVAL '1-2 3 4:5' MONTH)
  • Includes interval docs.

Note: I'm missing comprehensive unit tests, but will add those once I get a greenlight that the overall structure of this code looks good.

Quick addendum:

  • Postgres treats interval as having month, day, duration. However, I kept our implementation more similar to its prior iteration which only tracks month and duration, and treats day as part of duration. I haven't dug into ways in which this could interact with TIMESTAMP math, but it doesn't cause any regression in our current behavior.
  • In DateTimeField ranges, Postgres ignores the leading field (e.g. in MONTH TO MINUTE, MONTH is a noop). I instead let this define the most significant value to keep in the output. This seems useful but ¯_(ツ)_/¯--is very easy to yank out.
  • The previous code referred to all time-like strings as intervals, but the syntax for PG compatibility is slightly different, so I've tried to change the name to reflect that there are multiple time-like syntaxes. This naming and error messaging could be improved on, and would love any pointers here.

@quodlibetor
Copy link
Contributor

I haven't looked into the code yet, but re keeping days and durations in the same field: I believe that this should be the same for all of our times since we're UTC ignoring leap seconds, but when we start supporting timezones that have something like DST there will be a difference.

Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

I'm only about half way done but I thought I'd share the small things that I've got so far.

Overall this is looking amazing!

src/repr/scalar/mod.rs Show resolved Hide resolved
src/repr/scalar/mod.rs Show resolved Hide resolved
src/repr/scalar/mod.rs Outdated Show resolved Hide resolved
src/repr/scalar/mod.rs Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

🥇 oh boy.

You don't need to implement all my changes, but an explicit yup/nope would make it easier for me to think through what you have done/intend to do.

src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/ast/value/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/parser/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/parser/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/parser/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/src/parser/datetime.rs Outdated Show resolved Hide resolved
src/sql-parser/tests/sqlparser_common.rs Outdated Show resolved Hide resolved
src/sql-parser/tests/sqlparser_common.rs Show resolved Hide resolved
@sploiselle
Copy link
Contributor Author

@quodlibetor just wanted to give you a quick thanks and let you know I'll implement all of these comments this week. Appreciate your guidance on all these points; lots of things I wasn't aware of/old habits from Go.

@sploiselle
Copy link
Contributor Author

@quod_libetor Thanks for your patience on this. tl;dr I cleaned up everything you requested/didn't clean up anything we agreed to leave as-is.

Detailed notes:

  • Added thousands of lines' worth of unit tests!

    The implementation of this one type is knotted and spread out enough that I would love any new test failures to create a very solid trail of crumbs. I was potentially overzealous in the number of tests, so I could be convinced to pare these down.

    • I wasn't sure where to put all of the tests for IntervalValue, so just threw them at the bottom of the file; lmk if those should go elsewhere.
  • Availed to turn all comments into doc comments, which used more Rust doc idioms. However, there's a world in which I missed one or two. Please let me know if you see them.

  • Availed to reorg src/sql-parser/src/parser/datetime.rs to put "more important" functions at the top.

  • Added DateTimeFieldValue, which tracks the unit and fraction value in a struct. I considered having a field that controlled both field's sign, but that ended up being much fussier on reads (where people are less attentive).

  • Added the ability for TIMESTAMPS to have the same flexibility in their TIME component as SQL Standard-style H:M:S.NS (link). This only added 20 lines of code, and wanted to get it done while I saw the opening. Sorry! I did add .slt tests for the case.

Copy link
Contributor

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

💰💰💰💰💰💰💰💰

@sploiselle sploiselle merged commit 125d6fd into MaterializeInc:master Jan 14, 2020
@sploiselle sploiselle deleted the interval-pg-compat branch January 14, 2020 20:06
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.

None yet

2 participants