Skip to content
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

Update From false positive for multiple columns found with name #4777

Merged

Conversation

eygraber
Copy link
Contributor

@eygraber eygraber commented Nov 3, 2023

No description provided.

@griffio
Copy link
Contributor

griffio commented Nov 3, 2023

🔢 There are a few of these 'Multiple columns found with name...' issues #4767 (comment)
Maybe create this as an issue 🎫 as well

I was looking into these - but is quite complex 😵‍💫

In the PR above - it would seem that the sub query in the FROM is not flagged as adjacent to the UPDATE.

e.g UpdateStmtLimitedMixin - I think you can make the test pass by amending to return super.queryAvailable(child).map { it.copy(adjacent = true) } + joinClause!!.queryExposed()

Other source of error message is in sql-psi
https://github.com/AlecKazakova/sql-psi/blob/114fb05320f48b45c57583d41b3792567a9a0d34/core/src/main/kotlin/com/alecstrong/sql/psi/core/psi/SqlColumnReference.kt#L61

@eygraber
Copy link
Contributor Author

eygraber commented Nov 3, 2023

@griffio thanks for the pointer. Marking the results from super.queryAvailable would technically work, but I don't think those are adjacent in this case. Marking the joinClause!!.queryExposed() results as adjacent also works, and I think is more correct. Updating the PR now.

@eygraber eygraber force-pushed the sqlite-3_33-update-from-multiple-found-name branch 2 times, most recently from a2e7891 to 40a6d1f Compare November 3, 2023 19:51
@eygraber eygraber force-pushed the sqlite-3_33-update-from-multiple-found-name branch from 40a6d1f to 6afc5a0 Compare November 12, 2023 17:58
@hfhbd
Copy link
Collaborator

hfhbd commented Nov 15, 2023

Yes, using the joinClause is better.

@hfhbd hfhbd merged commit 9891ca5 into cashapp:master Nov 15, 2023
7 checks passed
hfhbd pushed a commit that referenced this pull request Apr 2, 2024
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.

None yet

3 participants