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

feat: Support for views #132

Merged
merged 30 commits into from
Oct 28, 2024
Merged

Conversation

Weijun-H
Copy link
Contributor

Ticket(s) Closed

What

CREATE VIEW AS SELECT ... FROM FOREIGN_TABLE do not push down to DuckDB

Why

How

In the process_utility_hook, push this view to DuckDB

Tests

test_view_foreign_table in tests/scan.rs

tests/scan.rs Outdated Show resolved Hide resolved
src/hooks/utility.rs Outdated Show resolved Hide resolved
@Weijun-H Weijun-H marked this pull request as ready for review September 27, 2024 03:01
@Weijun-H Weijun-H force-pushed the 54-Support-for-views branch 3 times, most recently from fd03179 to d470794 Compare September 27, 2024 04:39
@Weijun-H
Copy link
Contributor Author

Weijun-H commented Sep 27, 2024

---- test_view_foreign_table stdout ----
thread 'test_view_foreign_table' panicked at tests/scan.rs:615:9:
assertion `left == right` failed
  left: "error returned from database: column trips.vendorid does not exist"
 right: "WARNING: public.rate_code does not exist in DuckDB"

cannot reproduce the CI error on my M3 locally

@philippemnoel
Copy link
Collaborator

---- test_view_foreign_table stdout ----
thread 'test_view_foreign_table' panicked at tests/scan.rs:615:9:
assertion `left == right` failed
  left: "error returned from database: column trips.vendorid does not exist"
 right: "WARNING: public.rate_code does not exist in DuckDB"

cannot reproduce the CI error on my M3 locally

That's strange, I would expect this kind of error to not be environment-specific.

src/hooks/utility.rs Outdated Show resolved Hide resolved
src/hooks/utility.rs Outdated Show resolved Hide resolved
src/hooks/utility.rs Outdated Show resolved Hide resolved
src/hooks/utility.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@philippemnoel philippemnoel left a comment

Choose a reason for hiding this comment

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

This looks good to me! @kysshsy could you please help review here? If it's good with you, I'm happy to merge this.

@Weijun-H Weijun-H force-pushed the 54-Support-for-views branch 2 times, most recently from 765eff9 to 83cc9bd Compare October 14, 2024 12:41
src/hooks/utility/view.rs Outdated Show resolved Hide resolved
src/hooks/utility/view.rs Outdated Show resolved Hide resolved
@Weijun-H
Copy link
Contributor Author

Thanks for your feedback @kysshsy , I will rethink this pr

@Weijun-H Weijun-H marked this pull request as draft October 15, 2024 13:57
@Weijun-H Weijun-H marked this pull request as ready for review October 16, 2024 13:03
@kysshsy
Copy link
Contributor

kysshsy commented Oct 27, 2024

LGTM!

@philippemnoel philippemnoel merged commit a97c8f9 into paradedb:dev Oct 28, 2024
13 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.

Support for views
4 participants