Implement values

[spicychickensauce]

#138

I have no idea what I'm doing, but it seems to actually work and pass some tests!

I just tried to follow https://github.com/elixir-ecto/ecto_sql/blob/v3.13.2/lib/ecto/adapters/postgres.ex and adapt it to use a CTE combined with values as is needed in sqlite to be able to rename the table + the columns.

TODO:

• Use CAST where needed: Not needed for most things with sqlite, but probably needed for dates?
• Is the xxx temporary table name ok? Is there a better way? -> changed to random name
• Get all tests passing
• Test in my project for real world usage

[warmwaffles]

Before you get too deep on this, this is going to clash with anyone that uses with_cte/3

[spicychickensauce]

Why? Because of the hardcoded xxx name? If so, I don't think it's as bad as it seems, there is already a test where it uses 2 values(..) expressions and that test passes.
So I think it doesn't clash since the scopes are isolated.

Or is there another reason why it could clash with other CTEs?

[warmwaffles]

Well since you are lifting this into a with cte example. You'll need to make a test that has someone using both with_cte and values. I suspect, there will be a sql syntax error if used in combination. OR more concretely, using values twice in a statement.

[spicychickensauce]

Well since you are lifting this into a with cte example. You'll need to make a test that has someone using both with_cte and values. I suspect, there will be a sql syntax error if used in combination.

I'll definitely create a test for that. I think it works based on the already passing tests, but I'm not sure.

OR more concretely, using values twice in a statement

This is already proven to work, e.g. this test passes: https://github.com/elixir-ecto/ecto/blob/cd0f70b4cdd949767ea7cbe7d635e70917384b38/integration_test/cases/repo.exs#L2250

Btw, all tests with :values_list are passing, the only one that doesn't pass is this one: https://github.com/elixir-ecto/ecto/blob/cd0f70b4cdd949767ea7cbe7d635e70917384b38/integration_test/cases/repo.exs#L2266
But that one should be out of scope for this PR anyway, as it combines it with another feature that isn't supported.

[spicychickensauce]

I've added a test to see if there is any conflict with with_cte and there doesn't seem to be any.
Obviously the test is pretty bad, as it is a nonsense example, but I think it proves that there is no name collision even if a user names their table xxx as well.

I find this surprising as well, so maybe there would still be a more subtle issue with this...

[warmwaffles]

What does the SQL look like when you render run to_sql on it. I'm curious how it is shaped.

Like this:

ecto_sqlite3/test/ecto/adapters/sqlite3/connection/associations_test.exs

Lines 10 to 19 in 79d94c8

	test "association join belongs_to" do
	query =
	Schema2
	|> join(:inner, [c], p in assoc(c, :post))
	|> select([], true)
	|> plan()
	assert ~s|SELECT 1 FROM "schema2" AS s0 INNER JOIN "schema" AS s1 ON s1."x" = s0."z"| ==
	all(query)
	end

[warmwaffles]

I think you can shove this into the test/ directory under the connection/ directory as values_test

[spicychickensauce]

Inside test/ecto/adapters/sqlite3/connection?
If I do that I get ** (Exqlite.Error) no such table: posts. So there is some setup missing there.

Also, I didn't see any other similar tests in that directory, tests that actually operate on a Repo. Only in integration_test/

[spicychickensauce]

This tests fails with OTP 25, as the order of x, y is swapped in xxx(y, x).
This is because types is a map and therefore unordered.
It's not a bug, since if the order is different here, it will also be different later when passed as parameters. Both orders are correct and lead to the same query.

I'm not sure how to solve this, any idea?

[spicychickensauce]

What does the SQL look like when you render run to_sql on it. I'm curious how it is shaped.

Here is the formatted SQL output of Ecto.Adapters.SQL.to_sql(:all, TestRepo, query) for this query:

from(p in Post,
  right_join: params in values(params, types),
  on: params.post_id == p.id,
  left_join: c in Comment,
  on: c.post_id == p.id and fragment("LENGTH(?)", c.text) > params.n,
  group_by: params.id,
  select: {params.id, count(c.id)}
)

SELECT v1."id",
    count(c2."id")
FROM "posts" AS p0
    RIGHT OUTER JOIN (
        WITH xxx(id, n, post_id) AS (
            VALUES ($1, $2, $3),
                ($4, $5, $6)
        )
        SELECT *
        FROM xxx
    ) AS v1 ON v1."post_id" = p0."id"
    LEFT OUTER JOIN " comments " AS c2 ON (c2." post_id " = p0." id ")
    AND (LENGTH(c2." text ") > v1." n ")
GROUP BY v1." id "

[spicychickensauce]

I have tested this PR within my project with my actual use case and it passes all my tests ✔️

[spicychickensauce]

In the latest commit I changed the hardcoded xxx temp table name to temp_#{:rand.uniform(100_000)}.
This looks nicer + if there was any problem with reusing table names, then this would resolve that.

[spicychickensauce]

Seems like :location was not added yet in ExUnit v1.15.
Any idea how we could get this to work with elixir 1.15?

[spicychickensauce]

Would it be acceptable to skip all :values_list if running on Elixir v1.15?

[warmwaffles]

No ideas. But when elixir 1.19 is released, I am dropping support for 1.15. I could drop support for it now since I typically only like to support 3 versions back including current. 1.18, 1.17, 1.16 and otp 28, 27, 26

[spicychickensauce]

Ok, in that case I think it makes sense to not run those tests for 1.15. Then you can remove that once you decide to drop support. I've done that in the latest commit

[spicychickensauce]

I've added the type casting as in the postgres version.
It's probably only needed in a couple of cases, so it might do some unnecessary casting, but I don't think this will hurt.

Without it we were completely ignoring the types that the user supplied, so I think it makes more sense to just always cast.

[spicychickensauce]

I think this one is now ready. Sorry for the chaotic start, I was just excited that it worked at all...

[warmwaffles]

I'll take a look at this soon. I'm gonna check this out and see if I can come up with a breaking use case. It's hard to come up with concrete examples, but I'll see if I can blow it up. Overall, the main point I am having a hard time with is naming the cte selection as xxx. If there was a way to make it so the possibility of collision is impossible / more precise name like xxx1, xxx2, and so on (just like we do for i0, i1, u0, etc.. for table aliases.)

[spicychickensauce]

Overall, the main point I am having a hard time with is naming the cte selection as xxx

I've already addressed this in e851684, table names now contain a random number.

I did not want to enumerate the variable names, since that would have required introducing state to expr/1 and would have required a refactor. It would also have diverged more from the postgres implementation.

[spicychickensauce]

@warmwaffles I hate pinging, so sorry about that.
But it's been quite a while, I would appreciate any input on this.
Can I do anything to make it easier for a review?

[warmwaffles]

I looked at this over the weekend and I've decided I do not want to support this. I appreciate the work done, but I don't want to support this workaround to mimic what is possible with postgres. I would rather this adapter be as least clever as possible.

[spicychickensauce]

Damn, that is sad to hear.

I understand you not wanting to take on any more support burden, but I see no other place to put this code than here in the adapter.
I cannot implement this in user code via fragment, since it would require a dynamic amount of parameters.
values is the only ecto API that allows passing in a dynamic amount of parameters.
I do need to support sqlite besides postgres for my application, so I don't know how I can proceed.

Do you see any way how we could at least make it possible to then do this workaround via user land?

[warmwaffles]

So a work around would be to not do all the heavy lifting with a CTE. Pull those results in and process them in memory by the application. CTEs create temporary files in postgres, so they aren't exactly fast there either. CTEs are also glorified subqueries with named aliases for easier hand composing queries. Since we are using a query builder, it's not necessary to do that.

[spicychickensauce]

I really don't think I can do the join in elixir code, because I'm doing a conditional join based on what's in the values.
I would have to do N queries for every value.
I'll need some time to recheck my specific query, but I really don't think there is an acceptable workaround for my use case.

[warmwaffles]

There probably is a workaround that doesn't involve a CTE or pulling into the application. If you can accomplish the query with a CTE, you can accomplish it with a subquery.

[spicychickensauce]

@warmwaffles So while looking for an acceptable workaround, I have instead found a much better solution for the adapter. It no longer requires a CTE, no select * and no temp table names. Just a single select in a subquery.

I'm convinced that that was what I tried first, but I messed it up somehow and then trusted stackoverflow that a CTE would be the only way to implement it (https://stackoverflow.com/questions/43913457/how-do-i-name-columns-in-a-values-clause).

Here is a generated example query from my application with the new changes:

SELECT v1."id",
    count(distinct t2."id") + count(distinct w3."id")
FROM "artists" AS a0
    RIGHT OUTER JOIN (
        SELECT column1 as id,
            column2 as artist_id,
            column3 as loaded_at
        FROM (
                VALUES ($1::INTEGER, $2::INTEGER, $3::TEXT),
                    ($4::INTEGER, $5::INTEGER, $6::TEXT),
                    ($7::INTEGER, $8::INTEGER, $9::TEXT)
            )
    ) AS v1 ON v1."artist_id" = a0."id"
    LEFT OUTER JOIN "tracks" AS t2 ON (t2."deleted_at" IS NULL)
    AND (
        (t2."artist_id" = a0."id")
        AND (v1."loaded_at" < t2."inserted_at")
    )
    LEFT OUTER JOIN "web_tracks" AS w3 ON (w3."artist_id" = a0."id")
    AND (v1."loaded_at" < w3."inserted_at")
WHERE (a0."deleted_at" IS NULL)
GROUP BY v1."id"

I have 8 different instances of somewhat complicated queries that use VALUES and all my tests are passing.

I'll update the PR soon.
I hope with such a simplified implementation, you will accept the contribution.

[warmwaffles]

Oh this is significantly better. This actually implements the VALUES directive.

[warmwaffles]

I'll get to this later today.

[warmwaffles]

I'll cut a release for this later. There's some maintenance stuff I need to do as well for this.

[spicychickensauce]

Thanks @warmwaffles for taking the time for review and merge, much appreciated!

[warmwaffles]

There's some upstream issues with the integration tests that we run. I'll need to sort those out before I can release this. Not related to this PR change.

[warmwaffles]

@spicychickensauce released under v0.22.0

[spicychickensauce]

@warmwaffles Great, thank you very much! Back to mainland I go ⛵