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.

Sign up for GitHub

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

Jump to bottom

Fix: Date math with Interval keyword #12082

Merged
merged 3 commits into from
Jan 16, 2023
Merged

Fix: Date math with Interval keyword #12082

merged 3 commits into from
Jan 16, 2023

Conversation

GuptaManan100
Copy link
Member

Description

This PR fixes the issue described in #12076.
The queries which are valid but didn't work with Vitess have been added as tests and the parser has been patched.

Related Issue(s)

Checklist

  • "Backport to:" labels have been added if this change should be back-ported
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

@GuptaManan100
feat: add failing parsing test and fix parser
b0c92fa 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
test: add e2e tests for interval with math functions
fea6b52 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
Copy link
Member Author

I am unsure about the backport to any supported release. This syntax isn't really a regression, but it is still a bug fix.

@frouioui
Copy link
Member

This syntax isn't really a regression, but it is still a bug fix.

I think for this reason, let's not backport it @GuptaManan100.

@olyazavr
Copy link
Contributor

Would it be possible to backport this to v14? It has already appeared in a few places and broken people's queries, making it very difficult for us to upgrade to v14

@olyazavr
Copy link
Contributor

This works for our test cases (ported it over and ran vtcombo with the unit tests that were previously failing)

@GuptaManan100
Copy link
Member Author

@dbussink I am looking into the failure. This is very weird though. The test seems to work for me locally!

@GuptaManan100
Copy link
Member Author

Oh I just figured it out! Lol, the failure is because of different time zones. The UTC value is interpreted correctly, but the system time-zone is used to print the output. For me the timezone is IST which is +5:30, so it prints a time like 19:18:42, but the time-zone of the github-runner is different, so it prints 13:48:42 which corresponds to +0:00

@dbussink
Copy link
Contributor

Oh I just figured it out! Lol, the failure is because of different time zones. The UTC value is interpreted correctly, but the system time-zone is used to print the output. For me the timezone is IST which is +5:30, so it prints a time like 19:18:42, but the time-zone of the github-runner is different, so it prints 13:48:42 which corresponds to +0:00

Ah yeah, we probably have to assume UTC for things like this? And enforce that in the test?

@GuptaManan100
test: explictly set the time-zone to prevent failures in CI
b446b07 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100
Copy link
Member Author

I set the time-zone explicitly in the test now. Its a session setting which can be changed and this is something that Vitess allows to change too, so all good!

@GuptaManan100 GuptaManan100 merged commit 0bdc9b8 into vitessio:main Jan 16, 2023
@GuptaManan100 GuptaManan100 deleted the interval-math-expr branch January 16, 2023 11:31
@vitess-bot
Copy link
Contributor

vitess-bot bot commented Jan 16, 2023

I was unable to backport this Pull Request to the following branches: release-14.0, release-13.0, release-15.0.

GuptaManan100 added a commit to planetscale/vitess that referenced this pull request Jan 16, 2023
@GuptaManan100
Fix: Date math with Interval keyword (vitessio#12082)
4c9ec98 
* feat: add failing parsing test and fix parser

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add e2e tests for interval with math functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: explictly set the time-zone to prevent failures in CI

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 mentioned this pull request Jan 16, 2023
Merged
GuptaManan100 added a commit to planetscale/vitess that referenced this pull request Jan 16, 2023
@GuptaManan100
Fix: Date math with Interval keyword (vitessio#12082)
9aae61b 
* feat: add failing parsing test and fix parser

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add e2e tests for interval with math functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: explictly set the time-zone to prevent failures in CI

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 mentioned this pull request Jan 16, 2023
Merged
GuptaManan100 added a commit to planetscale/vitess that referenced this pull request Jan 16, 2023
@GuptaManan100
Fix: Date math with Interval keyword (vitessio#12082)
bfc8f6e 
Signed-off-by: Manan Gupta <manan@planetscale.com>
@GuptaManan100 GuptaManan100 mentioned this pull request Jan 16, 2023
Merged
@GuptaManan100
Copy link
Member Author

The backports are - #12102, #12103 and #12106

GuptaManan100 added a commit that referenced this pull request Jan 18, 2023
@GuptaManan100
Fix: Date math with Interval keyword (#12082) (#12102)
8024e6b 
* feat: add failing parsing test and fix parser

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add e2e tests for interval with math functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: explictly set the time-zone to prevent failures in CI

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
GuptaManan100 added a commit that referenced this pull request Jan 18, 2023
@GuptaManan100
[release-14.0] Fix: Date math with Interval keyword (#12082) (#12103)
9a5f084 
* Fix: Date math with Interval keyword (#12082)

* feat: add failing parsing test and fix parser

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add e2e tests for interval with math functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: explictly set the time-zone to prevent failures in CI

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: fix expected output

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
GuptaManan100 added a commit that referenced this pull request Jan 18, 2023
@GuptaManan100
Fix: Date math with Interval keyword (#12082) (#12106)
93c4150 
Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
dbussink pushed a commit that referenced this pull request Jan 30, 2023
@GuptaManan100
Fix: Date math with Interval keyword (#12082) (#1495)
2f608b7 
* feat: add failing parsing test and fix parser

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: add e2e tests for interval with math functions

Signed-off-by: Manan Gupta <manan@planetscale.com>

* test: explictly set the time-zone to prevent failures in CI

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>
@austenLacy austenLacy mentioned this pull request Apr 26, 2023
Merged
@austenLacy austenLacy mentioned this pull request Aug 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbussink dbussink dbussink approved these changes

@frouioui frouioui frouioui approved these changes

@harshit-gangal harshit-gangal Awaiting requested review from harshit-gangal harshit-gangal is a code owner

@systay systay Awaiting requested review from systay systay is a code owner

Assignees
No one assigned
Labels
Component: Query Serving Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug Report: Date math produces syntax error
4 participants
@GuptaManan100 @frouioui @olyazavr @dbussink