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

support join select #29

Merged
merged 3 commits into from
May 14, 2023
Merged

support join select #29

merged 3 commits into from
May 14, 2023

Conversation

valichek
Copy link
Contributor

@valichek valichek commented Mar 31, 2023

Please, review the changes, further directions required.

At the point this change will fix error from #26

ERROR: table `w` not available for query` for case
SELECT d.id, coalesce(w.w_count,0) as w_count FROM my_table AS d
LEFT JOIN LATERAL ( 
    SELECT count(*) as w_count FROM my_another_table WHERE d.id = ext_id AND deleted_at IS NULL
) w ON true
WHERE d.deleted_at IS NULL;

But another error comes out

table `d` not available for query

Following changes done in own commit being negotiable.
It looks like the only way to proceed is to pass tables used from top level here and join with ones parsed in validateSelectStmt(...)

The first thing that comes to my mind is to add top-level used tables to VetContext

type VetContext struct {
  Schema *schema.Db
  TablesUsed []TableUsed
}

It will go down through all the select statements in hierarchy. In order this to work properly used tables may be collected in ParsedResult.

Not much of test cases available in the project so maybe someone could point to the possible pitfalls of such decision.

@codecov-commenter
Copy link

Codecov Report

Patch coverage: 61.06% and project coverage change: -1.65 ⚠️

Comparison is base (f887b7a) 77.87% compared to head (c2ed7e2) 76.22%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master      #29      +/-   ##
==========================================
- Coverage   77.87%   76.22%   -1.65%     
==========================================
  Files           7        7              
  Lines         931      959      +28     
==========================================
+ Hits          725      731       +6     
- Misses        155      165      +10     
- Partials       51       63      +12     
Impacted Files Coverage Δ
pkg/vet/vet.go 78.43% <60.43%> (-3.40%) ⬇️
pkg/schema/postgres.go 63.15% <63.63%> (-0.95%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@houqp
Copy link
Owner

houqp commented Apr 4, 2023

The high level idea is we need to keep track of all the aliases definitions when parsing the queries, so I think your design is on the right track 👍

@valichek
Copy link
Contributor Author

valichek commented Apr 13, 2023

Latest commit introduced inner schema concept restricted to single query and protects original db schema from mutation. Also removed from Select queries call of getUsedTablesFromSelectStmt as redundant walking over the nodes to collect used tables - this moved to parseExpression(). If it looks fine we can proceed with more refactoring in validateUpdateStmt/validateInsertStmt,

@valichek valichek changed the title WIP: support join select support join select May 12, 2023
@valichek
Copy link
Contributor Author

@houqp I think this is ready

@houqp houqp merged commit 443693f into houqp:master May 14, 2023
@houqp
Copy link
Owner

houqp commented May 14, 2023

Thanks @valichek for the feature!

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