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

feat: add timestamp in max/min function #511

Merged
merged 1 commit into from
Jun 27, 2023

Conversation

icexelloss
Copy link
Contributor

@icexelloss icexelloss commented Jun 26, 2023

This PR add supports for timestamp types in max/min function.

This is currently supported in Acero and I believe most databases support this as well.

@CLAassistant
Copy link

CLAassistant commented Jun 26, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

ACTION NEEDED

Substrait follows the Conventional Commits
specification
for
release automation.

The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification.

@icexelloss icexelloss changed the title feat: Add timestamp in max/min function feat: add timestamp in max/min function Jun 26, 2023
Copy link
Member

@gforsyth gforsyth left a comment

Choose a reason for hiding this comment

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

This is supported in Acero and also in DuckDB (and likely others). Not a breaking change, so I think this can go in.

Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

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

The comparison functions (e.g. lt, gt, etc.) are defined with any so it seems we are already making an implicit assumption that all types are comparable amongst themselves. It seems, if comparison functions are defined, then max/min must exist as well. Perhaps it would make more sense to change min/max to use any instead of explicitly listing every type?

Looking at existing implementations we have:

Implementation Description of supported types
Postgres any numeric, string, date/time, network, or enum type, or arrays of these types
SQL Server MAX can be used with numeric, character, uniqueidentifier, and datetime columns, but not with bit columns.
Acero Non-nested types
MariaDb, Sqlite, Spark, Presto, DuckDb, Datafusion Does not list allowed types in documentation

That being said, this addition is pretty straightforward, so we can also defer this discussion for another day.

@westonpace
Copy link
Member

After sleeping on this I think the best course of action is to proceed with this PR as it is and we can address the any approach at some future date if needed.

@westonpace westonpace merged commit 6943400 into substrait-io:main Jun 27, 2023
14 of 16 checks passed
@icexelloss
Copy link
Contributor Author

Thanks @westonpace and @gforsyth !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants