Skip to content

Add support for AllTableColumns in PrimaryExpression #2197

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

Conversation

tomershay
Copy link
Contributor

PostgreSQL supports expressions such as posts.* (all columns from the posts table) in many places, such as the GROUP BY clause.

For example, in this query:

select c.post_id, p.* from posts p inner join comments c on c.post_id = p.post_id group by p.post_id, c.post_id, p.*;

This PR adds support for AllTableColumns in PrimaryExpression.

I wasn't sure if PrimaryExpression is the best fit for this, so will be happy for suggestions on relocating it if there's a better expression group for it.

@manticore-projects
Copy link
Contributor

@tomershay

Thank you for this contribution. My understanding is: A PrimaryExpression can be used anywhere and everywhere -- which is not the case for AllColumns or AllTableColumns and especially seems to depend on the RDBMS also. However, from a pragmatical view it comes close and indeed seem to make sense.

However, we segregate both Expression at other places already and I believe even have to use semantic lookaheads (which are expensive). So in my opinion, we either

a) keep segregating AllColumns or AllTableColumns wherever it is allowed (cleaner and faster) or
b) remove all those segregations in all productions and incorporate AllColumns or AllTableColumns into PrimaryExpression (slower, but more pragmatic)

What I will not want is a mix of both approaches because it can make the Grammar even harder to maintain in the future.

@tomershay tomershay force-pushed the tomer_shay_support_alltablecolumns_in_primary_expression branch from a0665a2 to 2e27ef1 Compare March 21, 2025 15:49
@tomershay
Copy link
Contributor Author

@manticore-projects, thanks, makes total sense.

remove all those segregations in all productions and incorporate AllColumns or AllTableColumns into PrimaryExpression (slower, but more pragmatic)

I went with the approach above, so I removed the segregation of AllColumns or AllTableColumns in other places, and added them just to PrimaryExpression.

Looking forward to hearing your thoughts.

@manticore-projects manticore-projects merged commit 9fd3b9f into JSQLParser:master Mar 21, 2025
3 of 4 checks passed
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.

2 participants