-
Notifications
You must be signed in to change notification settings - Fork 156
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
Conversation
ACTION NEEDED Substrait follows the Conventional Commits 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? |
Maybe try |
There was a problem hiding this 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?
Good question. Do you think it would make sense to add another impl to |
I think that would work yeah. |
There was a problem hiding this 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.
Fix spec violation described in #487 by
separating
strptime
intostrptime_timestamp
,strptime_date
, andstrptime_time
Here's the original PR adding this in case anyone wants to see the discussion
surrounding this addition: #272