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: timestamp inequality support #52

Merged
merged 11 commits into from
Jul 18, 2024

Conversation

Dustin-Ray
Copy link
Contributor

@Dustin-Ray Dustin-Ray commented Jul 17, 2024

Rationale for this change

Quick PR to add support for inequality expressions between timestamps. Since theyre represented internally as i64, they can be easily compared against one another.

What changes are included in this PR?

Adds support for statements with the following syntax:

SELECT * FROM table WHERE times <= timestamp '1970-01-01T00:00:00Z
SELECT * FROM table WHERE times >= timestamp '1970-01-01T00:00:00Z
SELECT * FROM table WHERE times > timestamp '1970-01-01T00:00:00Z
SELECT * FROM table WHERE times < timestamp '1970-01-01T00:00:00Z

Are these changes tested?

yes

@Dustin-Ray Dustin-Ray marked this pull request as ready for review July 17, 2024 03:12
@Dustin-Ray Dustin-Ray requested a review from iajoiner July 17, 2024 03:14
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Hmm where did you actually compare timestamps? Moreover how do you do it with different timezones?

@Dustin-Ray Dustin-Ray requested a review from iajoiner July 18, 2024 00:29
@Dustin-Ray
Copy link
Contributor Author

Hmm where did you actually compare timestamps? Moreover how do you do it with different timezones?

I would point you to this test here which is probably the most significant example, this test confirms that our approach matches that of postgres.

I have also added many other examples in this PR which test inequalities on various timezones as you pointed out coverage was lacking in those areas

@iajoiner
Copy link
Contributor

iajoiner commented Jul 18, 2024

@drcapybara BTW it will be really great if you use git rebase main and squash your commits. Ideally there should either be one commit per PR or the commits should have clear differentiation (e.g. add Bar for one and add Foo for another). If you check out how Linux kernel and Bitcoin community use git you will find out that they use this approach.
@dalbertom

@Dustin-Ray Dustin-Ray requested a review from iajoiner July 18, 2024 21:36
Copy link
Contributor

@iajoiner iajoiner left a comment

Choose a reason for hiding this comment

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

Really thanks for submitting and repeatedly modifying this PR! I really appreciate this!

@Dustin-Ray Dustin-Ray merged commit 255ebeb into main Jul 18, 2024
8 checks passed
@Dustin-Ray Dustin-Ray deleted the feat/timestamp-inequality-support branch July 18, 2024 21:41
Copy link

🎉 This PR is included in version 0.5.8 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

2 participants