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

Extended query tests #898

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cosminci
Copy link

Test parameterized w/ list fails.
It's equivalent to the second test which uses the legacy twiddle syntax.
The first test passes so the issue seems to appear when list (maybe other?) codec is involved.

@mpilquist
Copy link
Member

mpilquist commented Jun 13, 2023

I narrowed this down to the following example:

val params: continents.type *: Int *: EmptyTuple = continents *: 10_000_000 *: EmptyTuple

Or equivalently, after dealiasing *: and shapeless.HNil:

val params: continents.type :: Int :: HNil = continents :: 10_000_000 :: HNil

This fails with the following error:

ExtendedQueryTest.scala:58:50: type mismatch;
[error]  found   : rassoc$6.type (with underlying type List[String])
[error]  required: continents.type
[error]     val params: continents.type :: Int :: HNil = continents :: 10_000_000 :: HNil
[error]                 

We can work around this by manually specifying the type to cons:

val params: continents.type :: Int :: HNil =
    new ::[continents.type, Int :: HNil](continents, 10_000_000 :: HNil)

Or even letting those types be inferred while manually instantiating the cons constructor:

val params: continents.type :: Int :: HNil = new ::(continents, 10_000_000 :: HNil)

So this appears to be a bug in the Scala 2 compiler when handling right associative operators. Appears to be a manifestation of scala/bug#11117.

@milessabin
Copy link
Member

I don't think this is a compiler bug. The desugaring for right associated calls involves an assignment to an intermediate val (rassoc$6 in this case) and this is enough to break the identity you're expecting. You can reproduce something similar in both Scala 2 and 3,

miles@tarski:~% scala-cli --java-home /usr/lib/jvm/java-20-jdk
Welcome to Scala 3.3.0 (20, Java Java HotSpot(TM) 64-Bit Server VM).
Type in expressions for evaluation. Or try :help.

scala> val foo = new Object
val foo: Object = java.lang.Object@5cc1bf20

scala> val bar = foo
val bar: Object = java.lang.Object@5cc1bf20

scala> val baz: bar.type = foo
-- [E007] Type Mismatch Error: -------------------------------------------------
1 |val baz: bar.type = foo
  |                    ^^^
  |                    Found:    (foo : Object)
  |                    Required: (bar : Object)
  |
  | longer explanation available when compiling with `-explain`
1 error found

scala> val quux: foo.type = foo
val quux: foo.type = java.lang.Object@5cc1bf20

So, unfortunately the name (ie. the precise Symbol) that the value is presented as matters and has to match between the value and the singleton type.

@mpilquist
Copy link
Member

For completeness, here's the working test:

// Uses import skunk._ for *: and EmptyTuple polyfills; can replace with :: and HNil if preferred
sessionTest("parameterized w/ list") { s =>
  val continents = List("Europe", "Asia")
  val query = sql"""
    SELECT name, region FROM country
    WHERE continent IN (${varchar.list(continents)})
      AND population > $int4
  """.query(varchar *: varchar)

  val countryStream = for {
    preparedQuery <- Stream.eval(s.prepare(query))
    country <- preparedQuery.stream(new *:(continents, 10_000_000 *: EmptyTuple), chunkSize = 5)
  } yield country

  countryStream.compile.toList.map(_ => "ok")
}

@cosminci
Copy link
Author

Thanks for looking into this @mpilquist!

I've updated the test to use the explicit twiddle constructor.
Also changed the first test to use twiddles instead of cons and removed the shapeless._ import.

LMK if we want this merged, or rather add some examples to the docs, or both.

@cosminci
Copy link
Author

P.S.: If the list codec is not the first, new *: constructors need to be chained all the way up starting from it, even if the previous codecs are simple.

@cosminci
Copy link
Author

@mpilquist do we eventually want to merge this suite for regression purposes, or should I close the PR since it's achieved its purpose?

@mpilquist
Copy link
Member

Yeah, I think we should include.

@codecov-commenter
Copy link

Codecov Report

Merging #898 (cbc1515) into main (d21cffb) will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #898       /-   ##
==========================================
  Coverage   83.89%   83.94%    0.05%     
==========================================
  Files         125      125              
  Lines        1757     1757              
  Branches      211      211              
==========================================
  Hits         1474     1475        1     
  Misses        283      282       -1     

see 1 file with indirect coverage changes

@cosminci
Copy link
Author

My bad, should have ran the tests locally for both 2.13 and 3.

Compilations fails on Scala 3 with:

[error] -- Error: /home/runner/work/skunk/skunk/modules/tests/shared/src/test/scala/ExtendedQueryTest.scala:59:45 
[error] 59 |      country <- preparedQuery.stream(new *:(continents, 10_000_000 *: EmptyTuple), chunkSize = 5)
[error]    |                                             ^^^^^^^^^^
[error]    |     too many arguments for constructor *: in class *:: (): Any *: Tuple

Trying to find a workaround.

@cosminci
Copy link
Author

I couldn't find a way to please both Scala 2.13 and Scala 3 compilers.

val params: continents.type :: Int :: HNil =
    new ::[continents.type, Int :: HNil](continents, 10_000_000 :: HNil)

^ This isn't accepted by the Scala 3 compiler :(

[error]    |too many arguments for constructor *: in class *:: (): ((continents : List[String]), Int)

I don't feel like I have sufficient knowledge of the scala type system internals to understand what's happening here.

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

Successfully merging this pull request may close these issues.

4 participants