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

Pushing AsOf expressions down to tables used in unions and subqueries in view definitions #1159

Merged
merged 11 commits into from
Aug 8, 2022

Conversation

fulghum
Copy link
Contributor

@fulghum fulghum commented Aug 5, 2022

Tables used in unions and subqueries in a view definition weren't getting updated with an asof expression when one was used on the view. This PR extends the resolve_views analyzer rule to close those gaps.

Fixes: dolthub/dolt#4011

Added Dolt enginetests, too: dolthub/dolt#4028

…atements in views, since they don't automatically get included since Union is an Opaque node.
@fulghum fulghum requested a review from zachmu August 5, 2022 20:53
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a small simplification.

Also could use an engine test, check out TestVersionedViews

sql/analyzer/resolve_views.go Outdated Show resolved Hide resolved
sql/analyzer/resolve_views.go Outdated Show resolved Hide resolved
@zachmu
Copy link
Member

zachmu commented Aug 5, 2022

Also could use an engine test, check out TestVersionedViews

Actually you probably want VersionedQueries in queries.go

@fulghum fulghum changed the title Pushing AsOf expressions down to tables used in unions in a view definition Pushing AsOf expressions down to tables used in unions and subqueries in view definitions Aug 8, 2022
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

sql/analyzer/resolve_views.go Show resolved Hide resolved
enginetest/enginetests.go Show resolved Hide resolved
@fulghum fulghum merged commit 5ea178d into main Aug 8, 2022
@fulghum fulghum deleted the fulghum/issue-4011 branch August 8, 2022 23:53
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.

As of reach dependent on view commit history
2 participants