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

Generalize in_ #602

Closed
tysonzero opened this issue Jun 24, 2024 · 9 comments
Closed

Generalize in_ #602

tysonzero opened this issue Jun 24, 2024 · 9 comments

Comments

@tysonzero
Copy link

The current type is:

in_ :: (Functor f, Foldable f) => f (Field a) -> Field a -> Field SqlBool

But it seems like a nicer type would be:

in_ :: (Default EqPP fields fields, Foldable f) => f fields -> fields -> Field SqlBool

We can drop the Functor as the mapping is/can-be done after the toList/foldr type stuff, and we take advantage of the Default EqPP machinery that .=== uses since IN seems to work just fine in PostgreSQL on products/tuples.

This should close #404, #406, #419 and the issue is mentioned in open issues #431 and #599.

@tomjaguarpaw
Copy link
Owner

This seems like a good idea. Is it a breaking change? If so it would prefer to introduce it under a new name first, but I'm struggling to think of any code that could be broken by this.

@ocharles
Copy link
Collaborator

Anything that wraps in_ and has a type signature (myIn = in_) would break, because the type has changed. It's hypothetical, but probably a better path than trying to find a threshold of "realistic" breaks

@ocharles
Copy link
Collaborator

Oh wait, maybe I'm wrong because Field a has that instance? Might be worth checking that one,

@tysonzero
Copy link
Author

Yes Field a does have that instance. Should be a strict generalization, so only breakage I can think of would be if it leads to type ambiguity or if someone's using TypeApplications on it.

@tomjaguarpaw
Copy link
Owner

@tysonzero: can you confirm whether this does the job? #604

@tysonzero
Copy link
Author

@tomjaguarpaw

It'd be nice to drop the Functor constraint so it can work on Set and similar. Similarly in in_ the Functor constraint isn't even used at all, F.toList is called immediately on the input.

I'm not sure if there will be any performance difference using ors instead of the literal multi-column in operator, if the optimizer is smart in theory it hopefully will work the same, but worth considering.

tomjaguarpaw added a commit that referenced this issue Aug 29, 2024
Closes #404

Closes #602
@tomjaguarpaw
Copy link
Owner

Oh yeah, thanks. Please take another look: #604

(Regarding performance, let's leave that as a potential future optimization. I don't want to get into the weeds of that.)

@tysonzero
Copy link
Author

Then yes that seems reasonable!

@tomjaguarpaw
Copy link
Owner

Thanks for your input. If Opaleye contains blockers for you or even stuff that you find just unergonomic then please keep prompting me about it.

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

No branches or pull requests

3 participants