-
Notifications
You must be signed in to change notification settings - Fork 50
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
fix #318 #398
fix #318 #398
Conversation
Thanks for the PR, that is indeed solving the example that I opened in the issue, however I would like to challenge the fix (take it with a grain of salt though, I was not able to provide a solution myself), but does it make sense for to Regarding the scala implementation on the fix, see the comment on the code directly Thanks a lot |
@@ -570,8 +570,9 @@ object PrepareDynamicExecution { | |||
matchingExternals: List[External], | |||
secondaryLifts: List[Planter[_, _, _]] = List() | |||
): Either[String, (List[Planter[_, _, _]], List[Planter[_, _, _]])] = { | |||
val encodeablesMap = |
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.
I don't think that encodeableMap
and secondaryEncodeablesMap
should be declared here, this could be move after the declaration of the enum UidStatus
closer to where they are used.
lifts.map(e => (e.uid, e)).toMap | ||
|
||
var encodeablesMap = | ||
lifts.groupBy(_.uid) | ||
|
||
val secondaryEncodeablesMap = |
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.
We should also consider the secondaryEncodeablesMap
, I don't know exactly where this is coming from, but we should apply the same fix there (I think) I might have some time to find reproducer for a bug that involve the secondaryLifts
but for now I don't have one.
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.
I have found secondaryEncodeablesMap is from LiftQuery. and in LiftQuery situation, groupBy have bugs.:https://github.com/zio/zio-protoquill/actions/runs/7142963255/job/19453419575?pr=398
This patch could not solve the problem. I believe it would have a big patch to do. So this patch is not good perfect.
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.
Using the following test
"reproduce with liftQuery" in {
val repeated = Seq(Seq("a", "b"), Seq("c", "d"))
val ctx = new SqlMirrorContext(MirrorSqlDialect, Literal)
import ctx.*
val query = dynamicQuery[Custom].filter(v =>
repeated.map(l =>
quote(liftQuery(l).contains(v.name))
).reduce((l,r) => quote(l || r))
)
val sql = ctx.translate(query)
assert(sql == "SELECT v0.name FROM Custom v0 WHERE v0.name IN ('a', 'b') OR v0.name IN ('c', 'd')")
}
The secondaryLifts
parameters is always empty, so I'm not sure that it's related to liftQuery
(at least not in this), so I guess we should not touch it if we don't understand what it's related to
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.
According to fb14ec1#diff-0ed928e021d9d2075b190d1742422bf1f68d8a36d699741630f9fc4c0e1bb1d3R195
This is related to lift
inside liftQuery
and we should be able to reproduce/test with
liftQuery(...).foreach(p => query[P].filter(pq => pq.id == lift(foo)).updateValue(p))
However I'm not sure that this is valid Quill code
Edit: I could not let that go, so here is a way to reproduce it
"reproduce with additionalLifts" in {
val ctx: MirrorContext[MirrorSqlDialectWithReturnClause, Literal] = new MirrorContext[MirrorSqlDialectWithReturnClause, Literal](MirrorSqlDialectWithReturnClause, Literal)
import ctx.*
val nameFilter = Seq("John", "Doe")
case class Person(id: Int, name: String, age: Int)
val people = List(Person(1, "Joe", 123), Person(2, "Jill", 456))
val updateDynamic = quote {
(p: Person) => query[Person].filter(p =>
nameFilter.map(n => quote(sql"${p.name} == ${lift(n)}".asCondition)).reduce(_ || _)
).updateValue(p)
}
val mirror = ctx.run { liftQuery(people).foreach(p => updateDynamic(p)) }
assert(mirror.triple == ("UPDATE Person AS p SET id = ?, name = ?, age = ? WHERE p.name == ? OR p.name == ?", List(List(1, "Joe", 123, 36, "John", "Doe"), List(2, "Jill", 456, 36, "John", "Doe")), Dynamic))
}
However there is one big catch, it fails both with groupBy
and map(...).toMap
and this is because we are doing something a bit more complex here:
https://github.com/zio/zio-protoquill/blob/v4.8.0/quill-sql/src/main/scala/io/getquill/context/Particularize.scala#L252
This fix is pretty similar. but assumes as lot (i.e we can't refer to the same lift twice, which is probably wrong and should not be relied on):
val regularLiftIndexes =
regularLifts.groupBy(_.uid)
trace"Organizing into Lift Slots: ${slots}".andLog()
slots.foldLeft((List.empty[Planter[?, ?, ?]], Set.empty[Planter[?, ?, ?]])) {
case ((acc, viewed), LiftSlot.Numbered(valueClauseNum, uid)) =>
(acc :+ valueClauseLiftIndexes
.get(ValueLiftKey(valueClauseNum, uid))
.getOrElse {
throw new IllegalStateException(s"Could not find the Value-Clause lift index:${valueClauseNum},uid:${uid}. Existing values are: ${valueClauseLiftIndexes}")
}, viewed)
case ((acc, viewed), LiftSlot.Plain(uid)) =>
val planter: Planter[?, ?, ?] = regularLiftIndexes
.get(uid)
.flatMap(_.find(l => !viewed.contains(l)))
.getOrElse {
throw new IllegalStateException(s"Could not find the lift uid:${uid},uid:${uid}. Existing values are: ${regularLiftIndexes}")
}
(acc :+ planter , viewed + planter)
case (acc, other) =>
throw new IllegalStateException(s"Illegal LiftSlot: ${other}")
}._1
I think that however this highlight the real issue which is having lift with the same UID
val sortedEncodeables = | ||
uidsOfScalarTags | ||
.map { uid => | ||
encodeablesMap.get(uid) match { | ||
case Some(element) => UidStatus.Primary(uid, element) | ||
case None => | ||
case Some(head::Nil) => |
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.
Instead of having a var
we should convert that to a flatMap, that way there a no hidden "iterations", the issue with flatmap is that we would end up with duplications inside the final sortedEncodeables
which is not a good solution, and as far as I could tell order is important, so we can't use a Set
inside of a List
, however we could do the following:
val encodeablesMap =
lifts.groupBy(_.uid)
val secondaryEncodeablesMap =
secondaryLifts.groupBy(_.uid)
val sortedEncodeables =
uidsOfScalarTags.distinct
.flatMap { uid =>
encodeablesMap.get(uid) match {
case Some(elements) => elements.map(UidStatus.Primary(uid, _))
case None =>
secondaryEncodeablesMap.get(uid) match {
case Some(elements) => elements.map(UidStatus.Secondary(uid, _))
case None => UidStatus.NotFound(uid) :: Nil
}
}
}
It will be hard the measure the performance impact of this distinct
but I honestly don't think that it will matters a lot
Closing in favor of #520. See my notes there. |
Fixes #318
This just resolve some easy use cases.
but would also have problems when using liftQuery for this test case.