Proposal: IN and NOT IN fragment builders for product types with arbitrary arities
#2128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hmm.. CI has failed, but doesn't seem related to the PR to me 🤔 |
jatcwang
reviewed
Nov 3, 2024
| fs: F[A], | ||
| fallback: Fragment | ||
| )(implicit A: util.Write[A]): Fragment = { | ||
| if (A.length == 0) fallback |
There was a problem hiding this comment.
Thanks this looks good! Small suggestion - I don't think we need/should handle 0-arity products.
Reason:
- We no longer derive instances for case objects so it's really difficult to accidentally pass in a 0-arity product
- Which means the only other possible case is if user is passing
List[Unit], which never makes sense and it's better to generate a bad SQL that fails than silently fallback to TRUE/FALSE.
WDYT?
There was a problem hiding this comment.
Agree, I don't like it either. I'll update the PR, thx!
jatcwang
reviewed
Nov 4, 2024
| * @return | ||
| * the `IN` subquery expression or `FALSE` if `fs` is a 0-arity product. | ||
| */ | ||
| def inValues[F[_]: Reducible: Functor, A: util.Write](f: Fragment, fs: F[A]): Fragment = |
There was a problem hiding this comment.
I'm also wondering whether we can replace in with this. It should be source compatible
There was a problem hiding this comment.
Do you mean, remove the existing in/notIn whatsoever and rename the proposed inValues/notInValues to in/notIn, correct?
satorg
force-pushed
the
in-not-in-fragments
branch
from
61738cf
to
313e929
Compare
|
Hi @jatcwang ,
|
|
Looks great tyvm! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Current
in,notIn,inOpt, etc fragment builders only allow 1 or 2 values per matching row.The proposed builders take
Readinstead ofGetand therefore allow products of any arity.Moreover, the current 2-arity builders accept values packed as
Tuple2only. The proposed builders accept tuples as well as case classes, which can come in handy in some cases.Why
inValuesandnotInValuesnames?In fact, neither the existing nor the proposed builders allow to build full-featured
IN/NOT INexpression. Both allow just a subset of features – matching against a static set of values. Besides, we cannot haveinfunction overloads for both kinds of builders. Therefore, if we want to keep the existing builders, we'll have to choose another names for the new ones.Names
inValuesandnotInValueslook pretty reasonable, but I'm totally open to change them to something better.