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.

Sign up for GitHub

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

Jump to bottom

fix #318 #398

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix #318 #398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
167e404
fix #318
timzaak Dec 8, 2023
55e34f2
not a good patch
timzaak Dec 8, 2023
c69afa5
Merge branch 'master' into fix318
guizmaii Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions quill-sql/src/main/scala/io/getquill/context/QueryExecution.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,8 +570,9 @@ object PrepareDynamicExecution {
matchingExternals: List[External],
secondaryLifts: List[Planter[_, _, _]] = List()
): Either[String, (List[Planter[_, _, _]], List[Planter[_, _, _]])] = {
val encodeablesMap =
Copy link

@ex0ns ex0ns Dec 16, 2023 •
edited
Loading

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 =
Copy link

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.

Copy link
Contributor Author

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.

Copy link

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

Copy link

@ex0ns ex0ns Dec 16, 2023 •
edited
Loading

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

secondaryLifts.map(e => (e.uid, e)).toMap
Expand All @@ -594,13 +595,16 @@ object PrepareDynamicExecution {
case NotFound(uid) => s"NotFoundPlanter($uid)"
}
}

val sortedEncodeables =
uidsOfScalarTags
.map { uid =>
encodeablesMap.get(uid) match {
case Some(element) => UidStatus.Primary(uid, element)
case None =>
case Some(head::Nil) =>
Copy link

@ex0ns ex0ns Dec 16, 2023 •
edited
Loading

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

UidStatus.Primary(uid, head)
case Some(head::tails) =>
encodeablesMap += (uid, tails)
UidStatus.Primary(uid, head)
case _ =>
secondaryEncodeablesMap.get(uid) match {
case Some(element) => UidStatus.Secondary(uid, element)
case None => UidStatus.NotFound(uid)
Expand Down
24 changes: 24 additions & 0 deletions quill-sql/src/test/scala/io/getquill/issues/Issue318.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package io.getquill.issues

import io.getquill.*

case class Custom(name: String)



class Issue318 extends Spec {
"reproduce" in {
val nameFilter = Seq("a", "b")

val ctx = new SqlMirrorContext(MirrorSqlDialect, Literal)
import ctx._

val query = dynamicQuery[Custom].filter(v =>
nameFilter.map(name => quote(sql"${v.name} == ${lift(name)}".asCondition))
.reduce((l,r) => quote(l || r))
)
val sql = ctx.translate(query)
assert(sql.contains("'a'"))
assert(sql.contains("'b'"))
}
}