R: rel_to_sql() generates invalid SQL for r_dataframe_scan()

[krlmlr]

What happens?

To what extent is the SQL generated by rel_to_sql() supposed to be runnable? A simple example involving row_number() already gives parse errors.

To Reproduce

con <- DBI::dbConnect(duckdb::duckdb())
experimental <- FALSE
invisible(DBI::dbExecute(con, "CREATE MACRO \"==\"(a, b) AS a = b"))
df1 <- data.frame(a = 1)

rel1 <- duckdb:::rel_from_df(con, df1, experimental = experimental)
rel2 <- duckdb:::rel_project(
  rel1,
  list({
    tmp_expr <- duckdb:::expr_window(duckdb:::expr_function("row_number", list()), list(), list(), offset_expr = NULL, default_expr = NULL)
    duckdb:::expr_set_alias(tmp_expr, "___row_number")
    tmp_expr
  })
)
sql <- duckdb:::rel_to_sql(rel2)
writeLines(sql)
#> SELECT row_number(, NULL, NULL) OVER () AS ___row_number FROM (SELECT * FROM r_dataframe_scan(0x107deb930, (experimental = false))) AS dataframe_4427004208_1881778879
DBI::dbGetQuery(con, sql)
#> Error: Parser Error: syntax error at or near ","
#> LINE 1: SELECT row_number(, NULL, NULL) OVER () AS ___row_number ...
#>                           ^

^{Created on 2023-04-10 with reprex v2.0.2}

OS:

macOS aarch64

DuckDB Version:

7bdaddf8e4504405218c8521c7c38d9c3abf33f6

DuckDB Client:

R

Full Name:

Kirill Müller

Affiliation:

cynkra GmbH

Have you tried this on the latest master branch?

• I agree

Have you tried the steps to reproduce? Do they include all relevant data and configuration? Does the issue you report still appear there?

• I agree

[hannes]

It's supposed to be runnable so this is a bug

[Tmonster]

@krlmlr can this be closed?

[krlmlr]

I now see:

con <- DBI::dbConnect(duckdb::duckdb())
experimental <- FALSE
invisible(DBI::dbExecute(con, "CREATE MACRO \"==\"(a, b) AS a = b"))
df1 <- data.frame(a = 1)

rel1 <- duckdb:::rel_from_df(con, df1, experimental = experimental)
rel2 <- duckdb:::rel_project(
  rel1,
  list({
    tmp_expr <- duckdb:::expr_window(duckdb:::expr_function("row_number", list()), list(), list(), offset_expr = NULL, default_expr = NULL)
    duckdb:::expr_set_alias(tmp_expr, "___row_number")
    tmp_expr
  })
)
sql <- duckdb:::rel_to_sql(rel2)
writeLines(sql)
#> SELECT row_number() OVER () AS ___row_number FROM (SELECT * FROM r_dataframe_scan(0x1282ed580, (experimental = false))) AS dataframe_4969125248_1395266360
DBI::dbGetQuery(con, sql)
#> Error: Parser Error: syntax error at or near "x1282ed580"
#> LINE 1: ... FROM (SELECT * FROM r_dataframe_scan(0x1282ed580, (experimental = false))) AS...
#>                                                   ^

^{Created on 2023-05-11 with reprex v2.0.2}

I wonder if we can emit an inline table instead of r_dataframe_scan(), to generate something along the following lines:

SELECT row_number() OVER () AS ___row_number FROM (SELECT 1.0 AS a) AS dataframe_4969125248_1395266360

Bonus points if this supports data frames with zero or more than one row.

[Tmonster]

So this is because the query executes on a raw pointer for the data frame scan. If this could execute this would be bad for multiple reasons. I think we can close this issue and make another one focused on changing the default table name for data frame scans? Might also be good to discuss in our meeting.

You can also still generate valid & runnable sql if you create the table in the duckdb instance.

con <- DBI::dbConnect(duckdb::duckdb())
experimental <- FALSE
invisible(DBI::dbExecute(con, "CREATE MACRO \"==\"(a, b) AS a = b"))
dbExecute(con, "CREATE TABLE t1 (a int, b varchar);")
rel1 <- duckdb:::rel_from_table(con, "t1");
rel2 <- duckdb:::rel_project(
  rel1,
  list({
    tmp_expr <- duckdb:::expr_window(duckdb:::expr_function("row_number", list()), list(), list(), offset_expr = NULL, default_expr = NULL)
    duckdb:::expr_set_alias(tmp_expr, "___row_number")
    tmp_expr
  })
)
sql <- duckdb:::rel_to_sql(rel2)
writeLines(sql)
# SELECT row_number() OVER () AS ___row_number FROM MAIN.t1
DBI::dbGetQuery(con, sql)
# [1] ___row_number
# <0 rows> (or 0-length row.names)

[krlmlr]

This issue isn't about making r_dataframe_scan() work, it's about emitting alternative SQL for that call. Let's discuss.

[github-actions]

This issue is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[krlmlr]

Still interested in a solution here.

[hannes]

This is actually desired behaviour, we don't want to be able to create pointer values from SQL because of the security implications.

[hannes]

What we could do here is 1) enable a replacement scan for R data frames that looks in the enclosing scope for data frames with the table name and 2) override the ToString of r_dataframe_scan to have the name. However, I think the workaround that Tom provided is sufficient for a debugging feature.