Values clause does not support empty sequence

[NPCRUS]

I got an error: assertion failed: `Values` clause does not support empty sequence
when trying to do a Table.select.filter(t => db.values(ids).contains(t.id))
It's rather combersome, because i will need to eliminate all the branches with empty sequence myself.
Is it not supported by design or is it not supported YET ?

[NPCRUS]

one possible solution I see is method filterIf(ids.onEmpty)(t => db.values(ids).contains(t.id)) which would render expr from second argument only if Boolean from first evaluates to truer

[aboisvert]

I don't have a lot of insight on this but I'll share my thoughts:

• First of all, this was also surprising to me at first. And took a few repeated failures to avoid writing values(...) in cases where the argument could be empty.
• In most (~99%) of the cases, the queries boiled down to "return an empty set" if there were no values provided, so instead of wrapping values(...) into a safe utility method, the better choice for us was to skip doing a query entirely. (Most of our queries are user-facing so we optimize for latency and don't want to go out to the database if we know the result set will be empty).
• Knowing that values(...) fails with an empty Seq, I would prefer to have naming that reflects this ... something like valuesNonEmpty. Granted, it's a terrible name but it would effectively guards against the surprise effect.
• From a stylistic perspective, my preference would also be to have the operator be in instead of contains since that's typically the mindset that put ourselves into when writing SQL queries. e,g, .myColumn.in(matchingValues). (I'm aware of the irony of proposing both in and valuesNonEmpty in the same train of thought)

[NPCRUS]

So if values(Seq()) boils down to WHERE column in () it indeed renders query useless, I don't think anybody would want to implement it like that.
Instead I suggest we follow slick approach and do not render this part of the query at all
so that:
values(Seq()) => no changes to the query
values(Seq(1, 2)) => IN (1, 2)
@lihaoyi @aboisvert what do you guys think?

[aboisvert]

I couldn't find a case in our codebase where "erasing" the condition would be the desired semantic. If anything, I think it would be both more surprising than the current behavior and lead to subtle silent bugs.

Instead, if we want to avoid the surprise of a runtime exception, I would propose replacing values(Seq.empty)) by FALSE at the SQL level. I believe it would be the most logical substitution as I think it better matches expectation and the spirit of this conditional operator.

It is the equivalent of Set.empty contains anyValue, which always returns false.

[NPCRUS]

@aboisvert having a case class that represents some filters:

case class Filters(a: A, b: Seq[B])

Seq() represents no values passed for filtering by field b
seq if length > 0 represent at least one value passed for filtering
mind you i still need to filter by field a
now how would one translate this into scalasql query, maybe:

Table.select
  .filter(_.a === filters.a)
  .filter(t => db.values(filters.b).contains(t.b))

instead i need to do this:

val query1  = Table.select.filter(_.a === filters.a)
val query2 = if(filters.b.nonEmpty) {
  query1.filter(t => db.values(filters.b).contains(t.b))
else {
  query1
}

simple example, but if I will need to manually handle more dead branches, for example if a: A becomes a: Option[A](hinting lack of filterOpt)
this will become rather tedious and i will need to come up with my own extension methods which would do that
And in my codebases this happens all the time

[aboisvert]

We also do this (conditional filtering) a lot in our codebase, and we have an extension method called filterIf which I think would be a great addition to scalasql itself.

  extension [Q, R](select: Select[Q, R])
    /**
     * Filters this [[Select]] with the given query predicate, if the condition is true
     */
    def filterIf(condition: Boolean)(queryPredicate: Q => Expr[Boolean]): Select[Q, R] =
      if condition then select.filter(queryPredicate) else select

Taking your example as an illustration,

val query  = Table.select
  .filter(_.a === filters.a)
  .filterIf(filters.b.nonEmpty)(t => Values(filters.b).contains(t.b))

If this pattern is common enough for you, you can even shorten it to something like,

val query = Table.select
  .filter(_.a === filters.a)
  .filterIfContains(_.b, filters.b)

which would be another extension method or addition to the Select trait itself,

    /**
     * Filters this [[Select]] based on the given expression matching any of the given values.
     * Does not filter if values is empty.
     */
    def filterIfContains[T](expr: Q => Expr[T], values: Seq[T])(using
        Queryable.Row[T, T]
    ): Select[Q, R] =
      if values.isEmpty then select
      else select.filter(q => Values[T, T](values).contains(expr(q)))

(just a sketch, this may not compile or run correctly)

[NPCRUS]

@aboisvert I am 100% happy with having those as a helper methods on select in std. I guess PR will follow