Skip to content

Conversation

@romainfrancois
Copy link
Contributor

This uses the same approach as with alters strings, i.e. Elt() materialise all with VECTOR_ELT(AltrepVectorWrapper::Get(x)->Vector(), i);

there is probably a better way involving not forcing materialisation, i.e. an Elt() method that would just call duckdb_r_transform(chunk.data[column_index], dest, dest_offset, chunk.size(), false);

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Thanks, nice! Can you please add a test?

@codecov-commenter
Copy link

codecov-commenter commented Feb 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.93%. Comparing base (10b3416) to head (68460ac).

❗ Current head 68460ac differs from pull request most recent head 167880e. Consider uploading reports for the commit 167880e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #77      +/-   ##
==========================================
+ Coverage   85.89%   85.93%   +0.03%     
==========================================
  Files         106      106              
  Lines        3602     3612      +10     
==========================================
+ Hits         3094     3104      +10     
  Misses        508      508              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@romainfrancois
Copy link
Contributor Author

Will do. I just need to make the use case I started from independent of duckplyr, in test-list.R ?

library(duckdb)
library(duckplyr)

df <- tibble(a = list(1, c(1,2))) |>
  as_duckplyr_df()

filter(df, 0 == 0)
#> materializing:
#> ---------------------
#> --- Relation Tree ---
#> ---------------------
#> Filter [==(0.0, 0.0)]
#>   r_dataframe_scan(0x12b4a5e30)
#> 
#> ---------------------
#> -- Result Columns  --
#> ---------------------
#> - a (DOUBLE[])
#> 
#> # A tibble: 2 × 1
#>   a        
#>   <list>   
#> 1 <dbl [1]>
#> 2 <dbl [2]>

Created on 2024-02-22 with reprex v2.1.0

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 22, 2024

Try this:

duckdb <- asNamespace("duckdb")
con <- DBI::dbConnect(duckdb::duckdb(config = list(allow_unsigned_extensions = "true")))
experimental <- FALSE
invisible(
  DBI::dbExecute(con, "INSTALL 'rfuns' FROM 'http://duckdb-rfuns.s3.us-east-1.amazonaws.com'")
)
invisible(DBI::dbExecute(con, "LOAD 'rfuns'"))
invisible(DBI::dbExecute(con, "CREATE MACRO \"==\"(x, y) AS \"r_base::==\"(x, y)"))
df1 <- tibble::tibble(a = list(1, c(1,2)), b = 1)

rel1 <- duckdb$rel_from_df(con, df1, experimental = experimental)
rel2 <- duckdb$rel_filter(
  rel1,
  list(
    duckdb$expr_function(
      "==",
      list(
        duckdb$expr_reference("b"),
        if ("experimental" %in% names(formals(duckdb$expr_constant))) {
          duckdb$expr_constant(1, experimental = experimental)
        } else {
          duckdb$expr_constant(1)
        }
      )
    )
  )
)
rel2
duckdb$rel_to_altrep(rel2)

I have infrastructure to generate those.

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 22, 2024

Even simpler:

duckdb <- asNamespace("duckdb")
con <- DBI::dbConnect(duckdb::duckdb())
experimental <- FALSE
df1 <- tibble::tibble(a = list(1, c(1,2)), b = 1)

rel1 <- duckdb$rel_from_df(con, df1, experimental = experimental)
rel2 <- duckdb$rel_filter(
  rel1,
  list(
    if ("experimental" %in% names(formals(duckdb$expr_constant))) {
      duckdb$expr_constant(TRUE, experimental = experimental)
    } else {
      duckdb$expr_constant(TRUE)
    }
  )
)
rel2
duckdb$rel_to_altrep(rel2)

@krlmlr
Copy link
Collaborator

krlmlr commented Feb 22, 2024

In a duckdb test, we don't need duckdb$, of course.

@romainfrancois
Copy link
Contributor Author

I added a test in test_list.R but I get the feeling it might belong to test-rel_api.R instead ?

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Tests now fail if ALTREP is not supported. Can you please add conditional compilation?

df1 <- tibble::tibble(a = list(1, c(1,2)))

rel1 <- rel_from_df(con, df1)
rel2 <- rel_filter(rel1, list(expr_constant(TRUE)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, sorry for the confusion. I'm not sure we even need the filter here for the roundtrip, but it doesn't hurt.

Copy link
Collaborator

@krlmlr krlmlr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@krlmlr krlmlr requested a review from hannes February 23, 2024 15:15
@krlmlr krlmlr marked this pull request as ready for review February 23, 2024 15:16
@krlmlr krlmlr changed the title initial altlist support for LIST logical type feat: Initial ALTREP support for LIST logical type Feb 23, 2024
@krlmlr krlmlr added this to the 0.10.0 milestone Feb 28, 2024
@krlmlr krlmlr merged commit 9b0f53f into duckdb:main Mar 3, 2024
@krlmlr
Copy link
Collaborator

krlmlr commented Mar 3, 2024

Thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 4, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants