Add support for cross lateral joins.

[wilkinson4]

No description provided.

[greg-rychlewski]

@wilkinson4 are you able to point this at your ecto PR branch? i think integration tests will fail for tds/mysql. maybe we can add it for those adapters too since it's a small change

[greg-rychlewski]

I also think we have to change join_on https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/postgres/connection.ex#L512

I don't think cross lateral join uses ON, same a cross join.

[wilkinson4]

@wilkinson4 are you able to point this at your ecto PR branch? i think integration tests will fail for tds/mysql. maybe we can add it for those adapters too since it's a small change

@greg-rychlewski I am trying to point it at my ecto PR now, but I've never done that before so I'm still looking into that. Added the join_on case for postgres, but I think that cross lateral join is only supported by Postgres presently. Would that mean this change would require more work to make cross join lateral only available for Postgres Repo's?

[greg-rychlewski]

Oh so in your mix.exs you just do this: {:ecto, git: "https://github.com/your_github_name/ecto.git", branch: "your_pr_branch"}. Then do mix deps.get ecto or mix deps.update ecto (i can't remember which one) to update mix.lock.

I'm pretty sure MySQL has cross lateral join as CROSS JOIN LATERAL. TDS I'm not sure about though. The statements in the adapter for lateral joins are a bit unusual: https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/tds/connection.ex#L527.

Unless Jose knows, maybe just ignore the cross_lateral tag in TDS's integration test. They can be ignored here: https://github.com/elixir-ecto/ecto_sql/blob/master/integration_test/tds/test_helper.exs#L4

[greg-rychlewski]

Would that mean this change would require more work to make cross join lateral only available for Postgres Repo's?

Not too much. We could just raise a nice error like this in the join_qual function: https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/tds/connection.ex#L60

[greg-rychlewski]

Actually lateral join only works in MySQL from 8.0.14 and the CI is on 5.7 only. So I guess MySQL needs to ignore the integration test as well. I'll test it out on my local though to make sure it works on MySQL 8.

[wilkinson4]

Oh so in your mix.exs you just do this: {:ecto, git: "https://github.com/your_github_name/ecto.git", branch: "your_pr_branch"}. Then do mix deps.get ecto or mix deps.update ecto (i can't remember which one) to update mix.lock.

I'm pretty sure MySQL has cross lateral join as CROSS JOIN LATERAL. TDS I'm not sure about though. The statements in the adapter for lateral joins are a bit unusual: https://github.com/elixir-ecto/ecto_sql/blob/master/lib/ecto/adapters/tds/connection.ex#L527.

Unless Jose knows, maybe just ignore the cross_lateral tag in TDS's integration test. They can be ignored here: https://github.com/elixir-ecto/ecto_sql/blob/master/integration_test/tds/test_helper.exs#L4

@greg-rychlewski Temporarily changed the ecto_dep path to my forked ecto repo.

[greg-rychlewski]

@wilkinson4 it looks like there are some failures. for the integration test I think it's because you need to use a subquery similar to the change you made in the unit test

[greg-rychlewski]

If you're able to, it's really easy to run the tests locally so you can make sure they're passing. For unit tests just do ECTO_PATH=local_path_to_your_ecto mix test

For integration tests do ECTO_PATH=local_path_to_your_ecto ECTO_ADAPTER=pg mix test

[greg-rychlewski]

You're getting a bunch of weird errors that look unrelated to your change. It could be because your ecto branch is 182 commits behind master but your ecto_sql branch is up to date. Are you able to bring your ecto branch up to date?

[wilkinson4]

You're getting a bunch of weird errors that look unrelated to your change. It could be because your ecto branch is 182 commits behind master but your ecto_sql branch is up to date. Are you able to bring your ecto branch up to date?

@greg-rychlewski Okay. Sorry about all the changes needed. I synced my ecto fork and rebased master onto my ecto PR branch so that is up to date now. I also added the errors for tds and myxql adapters, and fixed the broken Postgres test. All of the Postgres tests are passing for me locally, but I am having issues with the TDS and MySQL tests locally as I don't have those databases installed.

[greg-rychlewski]

No worries. Ok so just a couple small things then we can merge once the Ecto change is merged:

1. Can you please remove the mix.lock change. Then when the Ecto PR is merged update it with that commit
2. Can you please add cross_lateral support for mysql? Both the join_qual and join_on function. This way people using version 8+ can use it. It works, this is from my local:

mysql> select * from test t1 cross join lateral (select * from test) t2;
+------+------+
| i    | i    |
+------+------+
|    2 |    1 |
|    1 |    1 |
|    2 |    2 |
|    1 |    2 |
+------+------+

3. One small nit about the error for TDS

[wilkinson4]

No worries. Ok so just a couple small things then we can merge once the Ecto change is merged:

1. Can you please remove the mix.lock change. Then when the Ecto PR is merged update it with that commit

2. Can you please add `cross_lateral` support for mysql? Both the `join_qual` and `join_on` function. This way people using version 8+ can use it. It works, this is from my local:

mysql> select * from test t1 cross join lateral (select * from test) t2;
+------+------+
| i    | i    |
+------+------+
|    2 |    1 |
|    1 |    1 |
|    2 |    2 |
|    1 |    2 |
+------+------+

3. One small nit about the error for TDS

@greg-rychlewski Made all of the changes to this PR and the ecto PR. Should be good to run the tests again.

[greg-rychlewski]

Thanks @wilkinson4 looks great! Just need to update mix.lock with the Ecto commit we just merged then it's good to go :).

[wilkinson4]

@greg-rychlewski You want me to add this: {:ecto, git: "https://github.com/elixir-ecto/ecto"} and commit the mix.lock file?

[greg-rychlewski]

Sorry I wasn't very clear. Please change mix.exs to this

{:ecto, "~> 3.10.0-dev", github: "elixir-ecto/ecto"}

Then mix.update ecto and commit both mix.exs and mix.lock

[greg-rychlewski]

Thank you! These are nice changes.

[wilkinson4]

Thank you! These are nice changes.

Thank you for all your help!