-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
add generate_series for timestampTZ values #64887
Conversation
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing: 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
Thanks for the contribution! If you can, it would be great to also add a Could you add a test like the one below (inspired from #64722)?
These two statements should return the same results (might need to adjust the end time so they return the same number of rows). |
sure, will do! |
@RaduBerinde isn't the |
fcb7d81
to
42144c5
Compare
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
I added the test case that you suggested |
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.
Ah, sorry, I guess it is already implemented.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @oeph, @otan, and @yuzefovich)
pkg/sql/logictest/testdata/logic_test/builtin_function, line 3021 at r1 (raw file):
SET TIME ZONE +0 statement ok
This only tests that we don't crash.. We should use query
to see the output so we can confirm that the two are equivalent.
@RaduBerinde is there a way to automatically compare the outputs of both query to match? I can't find any for the sqllogictest - can you maybe give me a hint? Or should I just add these both queries as standalone queries? |
42144c5
to
d950d65
Compare
Ah, it's not necessary, we can just visually compare. We can leave a comment saying that the results should be the same if they ever were to change. We review any changes to these files. |
This adds `generate_series(timestamptz, timestamptz, interval)` to the generate_series functions Release note (sql change): adds generate_series for timestamptz values
d950d65
to
723ded9
Compare
@RaduBerinde I added a comment for this |
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.
bors r+
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @oeph, @otan, and @yuzefovich)
Build succeeded: |
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.
thanks @oeph !
This change adds
generate_series(timestamptz, timestamptz, interval)
as it is also supported by postgresFixes #64850