-
Notifications
You must be signed in to change notification settings - Fork 15
feat(r): Generic datasources #28
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
base: main
Are you sure you want to change the base?
Conversation
...instead of requiring explicit DataSource subclass creation
…provements Plus some improvements: - Cleaner .md file reading code in example apps - Use GPT-4.1 by default, not GPT-4 😬 - Make sqlalchemy required
…-datasource-improvements
fix: No longer need to manually calls session$ns() with shinychat (#1…
(Changing the target of this to main so it merges cleanly) |
I'm really sorry, this is still waiting on me, isn't it? I'm going to book some time for us to discuss in realtime if that's cool with you. |
Alright, I think this is ready to merge @jcheng5 |
Previously, the examples/app-database.R would shown an error on startup because the initial query was "", which was then sent as a SQL query to RSQLite. The get_lazy_data code path accounted for the "" query, so we decided to make the eager code path just call the lazy code path, then collect(). Also fixed a formatting issue with the table.
It seems like dbplyr tables-as-queries can be a bit... temperamental. This should fix that by explicitly declaring sql always.
Shooot, there is one problem left. We're now using
I'll ask Hadley what the right way to do this is (if there is one). |
This is a WIP but seems to basically work.
One question -- I retained the basic functionality for local data.frames (that is, df -> querychat -> df), where remote data sources instead return a dbplyr lazy
tbl()
, meant for chaining. Is this too confusing of a behavior split? Should local data.frames also return atbl()
, just now connected to duckdb?A few immediate TODOs: