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: Initial ALTREP support for LIST logical type #77

Merged
merged 5 commits into from Mar 3, 2024

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?

src/reltoaltrep.cpp Outdated Show resolved Hide resolved
@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.

src/reltoaltrep.cpp Outdated Show resolved Hide resolved
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
18 checks passed
@krlmlr
Copy link
Collaborator

krlmlr commented Mar 3, 2024

Thanks!

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.

None yet

3 participants