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.