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

fix: separate strptime to fix spec violation #493

Merged
merged 1 commit into from
Apr 11, 2023

Conversation

richtia
Copy link
Member

@richtia richtia commented Apr 10, 2023

Fix spec violation described in #487 by
separating strptime into strptime_timestamp, strptime_date, and strptime_time

Here's the original PR adding this in case anyone wants to see the discussion
surrounding this addition: #272

@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.

@richtia richtia changed the title fix: separate stptime to fix spec violation fix: separate strptime to fix spec violation Apr 10, 2023
@richtia richtia requested a review from westonpace April 10, 2023 14:36
@richtia
Copy link
Member Author

richtia commented Apr 10, 2023

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.

Hmm...not actually sure why i'm seeing this?

@rok
Copy link
Contributor

rok commented Apr 10, 2023

Hmm...not actually sure why i'm seeing this?

Maybe try fix(extension): separate stptime to fix spec violation?

Copy link
Contributor

@rok rok left a comment

Choose a reason for hiding this comment

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

What about when timezone is present when parsing time and date? Would we want to return those in UTC or local time?

@richtia
Copy link
Member Author

richtia commented Apr 10, 2023

What about when timezone is present when parsing time and date? Would we want to return those in UTC or local time?

Good question. Do you think it would make sense to add another impl to strptime_time and strptime_date that has timezone as a parameter and behave similarly to strptime_timestamp?

@rok
Copy link
Contributor

rok commented Apr 10, 2023

I think that would work yeah.
We could probably just merge this change as is to fix the current violation and add other variants in separate PR or when needed.

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.

I agree this is a good change and that impls cannot differ only by return type. We were violating this rule:

It is required that two function implementations with the same simple name must resolve to different compound names using types. If two function implementations in a YAML file resolve to the same compound name, the YAML file is invalid and behavior is undefined.

Given the previous version was not valid I don't think this will count as a backwards incompatible change and we should merge it so we can get moving forwards.

I'll CC @jacques-n and @cpcloud for a posthumous review if/when they have time.

@westonpace westonpace merged commit 8c230af into substrait-io:main Apr 11, 2023
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.

3 participants