Skip to content

feat: add Query.table for joins directly on a table - #11

Open
tjdevries wants to merge 1 commit into
kiranandcode:masterfrom
tjdevries:table-joins
Open

tjdevries wants to merge 1 commit into
kiranandcode:masterfrom
tjdevries:table-joins

Conversation

@tjdevries

@tjdevries tjdevries commented Aug 9, 2023

Copy link
Copy Markdown

Hi!

First off, really enjoying petrol. Thanks for the library.

I was wondering if this would be a welcome addition to the library. I'm not necessarily 100% sold on using TABLE as a new variant, but I wasn't sure how else to handle this easily (and also allows inserting tables into other locations in queries later, if needed).

Usage looks now like this:

    Query.select ~from:table Expr.(User.f_twitch_display_name :: fields)
    |> Query.join
         ~op:Query.INNER
         ~on:Expr.(User.f_twitch_user_id = f_user_id)
         (Query.table User.table)

Printed:

SELECT users.twitch_display_name, suggestions.id, ...
FROM suggestions
   INNER JOIN users ON users.twitch_user_id = suggestions.user_id

Thanks

@kiranandcode

Copy link
Copy Markdown
Owner

Ohh, nice this looks interesting! Thanks so much for your PR. This change makes a lot of sense... I had noticed a lot of repetition when doing joins with tables where I was using all the columns.

My only hesitation is that the last time I was working on the code, I remember making a quick hack to make sub-queries work on postgres involving automatically giving names to sub-queries (https://github.com/Gopiandcode/petrol/blob/e83d3c833b98a3a915949651316a04469417d7f5/lib/types.ml#L276), which I think is the cause of some issues discussed in #3.

I was thinking of making some kind of large change to allow representing table aliases (JOIN ... AS B) explicitly in the DSL.
It may be the case that this kind of change may subsume this one.

@tjdevries

Copy link
Copy Markdown
Author

I was thinking of making some kind of large change to allow representing table aliases (JOIN ... AS B) explicitly in the DSL.
It may be the case that this kind of change may subsume this one.

No problem :) it was a fun PR. I don't know that the subquery problem will exist because there isn't a subquery when using the table this way (it's just a join on a table, so the resulting query should work fine in postgres or not, at least that is my understanding).

That's why I have this part:

https://github.com/Gopiandcode/petrol/pull/11/files#diff-c6395e65757951e5358bc2a6170b82ae6bf1ef444258b3918fb58a5fa5394245R280-R290

I think it should avoid the problem.

Either way, thanks for the response :)

@royneary

Copy link
Copy Markdown
Contributor

I was thinking of making some kind of large change to allow representing table aliases (JOIN ... AS B) explicitly in the DSL.

Hi @kiranandcode, if you still remember, could you elaborate? I'd like to look into it, but I'm a bit clueless, how table aliases could be added to the DSL.

@kiranandcode

Copy link
Copy Markdown
Owner

@royneary I don't have a great recollection of how that would work, but if I had to guess we could potentially add a "Named" term to the DSL which is constructed by a helper function named (<sql-query>) ~_as:"..." and then in the prettyprinting of the dsl expressions if we see a named expression then we print accordingly?

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.

3 participants