feat: With duckdb(environment_scan = TRUE), data frame objects are available as views in duckdb SQL queries
#164
Conversation
|
Needs still tests and some protect stuff I guess |
src/register.cpp
Outdated
| return nullptr; | ||
| } | ||
| if (TYPEOF(df) == PROMSXP) { | ||
| df = Rf_eval(df, rho); |
There was a problem hiding this comment.
cpp11 has a global protection mechanism, but how to control the lifetime?
There was a problem hiding this comment.
indeed lifetime is a problematic issue. For example, once someone creates a view of this and then deletes the df and gc runs we might be in trouble.
There was a problem hiding this comment.
IIUC, support for lifetime control has landed via duckdb/duckdb#11761, which is why we now have conflicts here?
There was a problem hiding this comment.
We already had support for lifetime control, but it was broken in a couple ways
We use the ExternalDependency class for lifetime control, but this wasn't being kept alive long enough, which this PR aimed to address.
We changed the prototype of the replacement scan, we changed the const string &table_name into ReplacementScanInput
I have a follow up PR scheduled that removes the newly added table_ref &TableRef from this struct though
Instead we will collect replacement scans inside QueryRelation::Bind (virtual overload) and push these replacement scans as CTEs onto the parsed QueryNode stored inside the QueryRelation.
There was a problem hiding this comment.
Thanks, Thijs. May I get back to you for help with lifetime control after that follow-up PR has made it into the R package? I don't find that PR in https://github.com/duckdb/duckdb/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+author%3ATishj.
There was a problem hiding this comment.
@Tishj: how would I go about controlling the lifetime of the bound object in this PR?
| return nullptr; | ||
| } | ||
| // TODO: do utf conversion | ||
| auto table_function = make_uniq<TableFunctionRef>(); |
There was a problem hiding this comment.
Or perhaps subclass TableFunctionRef ?
|
@krlmlr can you get this merged? I think the secondary lifetime issues might be something for a follow-up PR |
|
Sure! I'd like to make it "opt in" though, otherwise it may (and probably will) break user code. |
Is it really so? |
|
Right, we discussed that. For 1.1.3-1, I'd make it opt-in, we can make it opt-out later. |
krlmlr
force-pushed
the
replacementscandf
branch
from
0fed172 to
2ce02b5
Compare
krlmlr
changed the title
duckdb(environment_scan = TRUE), data frame objects are available as views in duckdb SQL queries
…available as views in duckdb SQL queries (#164) Co-authored-by: Hannes Mühleisen <hannes@duckdblabs.com> Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com> Co-authored-by: Kirill Müller <kirill@cynkra.com>
…available as views in duckdb SQL queries (#164) Co-authored-by: Hannes Mühleisen <hannes@duckdblabs.com> Co-authored-by: Kirill Müller <krlmlr@users.noreply.github.com> Co-authored-by: Kirill Müller <kirill@cynkra.com>
|
Is there a reprex for how should this work? I can't seem to get this to work even on duckdb_1.1.3-1 |
|
There are tests. |
|
cc @viktorleis |
now "just" works
CC @krlmlr
Closes #140.