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

Add between #127

Merged
merged 7 commits into from
Aug 8, 2019
Merged

Add between #127

merged 7 commits into from
Aug 8, 2019

Conversation

ibarrae
Copy link
Contributor

@ibarrae ibarrae commented Jun 26, 2019

Between was added by myself on #111 but it was reverted on #123 due to the fact that new between clause was not working with non-val values as @parsonsmatt mentioned here.

I changed that and also included some tests to cover that case.

Please, let me know if there's another missing case so I can also address it 😃

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

I think we should support composite keys instead of throwing an error. Otherwise this looks fantastic, thanks! 😄

src/Database/Esqueleto/Internal/Internal.hs Outdated Show resolved Hide resolved
src/Database/Esqueleto/Internal/Internal.hs Outdated Show resolved Hide resolved
test/Common/Test.hs Outdated Show resolved Hide resolved
@ibarrae
Copy link
Contributor Author

ibarrae commented Jun 27, 2019

Hey, @parsonsmatt I just pushed some changes but it maintains BetweenError because I figured out while doing some tests that when you mix scalar and composite values it also fails on different databases (using between and >= <= accordingly ) and would lead to runtime errors, so I think it's better to throw an error if this scenario occurs.. Summing up, it works when all values for between are scalar or composite only, but it cannot be scalar and composite. Let me know what you think of this.

mysql> select * from point where ((x, y) >= (1) and (x,y) <= (3,4));
ERROR 1241 (21000): Operand should contain 2 column(s)
postgres> select 1 from table where (1, 2) >= 1 and (1, 2) <= (2,3);
select 1 from table where (1, 2) between 1 and (3, 4); 
ERROR:  operator does not exist: record >= integer

@ibarrae
Copy link
Contributor Author

ibarrae commented Jun 27, 2019

I updated unsafeSqlBinOp, would you mind taking a look again @parsonsmatt

@ibarrae
Copy link
Contributor Author

ibarrae commented Aug 2, 2019

Hey @parsonsmatt I think I got this correctly this time, could you take a look when possible, please and also, CI seems to be failing due to a time out, could you restart it if possible too?

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

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

Approved, pending the @since modification and a CHANGELOG entry please 😄

src/Database/Esqueleto/Internal/Internal.hs Outdated Show resolved Hide resolved
test/Common/Test.hs Show resolved Hide resolved
test/Common/Test.hs Show resolved Hide resolved
@ibarrae
Copy link
Contributor Author

ibarrae commented Aug 8, 2019

@parsonsmatt Done! I updated the version number and updated the changelog entry, thanks a lot for your help in this 😄

@parsonsmatt
Copy link
Collaborator

Thank you! You've written a new feature, discovered a bug, and fixed that bug all in one go 😄

@parsonsmatt parsonsmatt merged commit 5d8f5b5 into bitemyapp:master Aug 8, 2019
@ibarrae ibarrae deleted the add-between branch August 8, 2019 18:33
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