Skip to content

Add additional extension URI to Arrow Substrait Compiler #208

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

Merged
merged 29 commits into from
Nov 24, 2022

Conversation

thisisnic
Copy link
Contributor

@thisisnic thisisnic commented Nov 15, 2022

Fixes #202

See the unskipped function in this PR (which uses the comparison function "gt", which we hadn't previously implemented here) to see this working.

Copy link
Contributor

@paleolimbot paleolimbot left a comment

Choose a reason for hiding this comment

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

A few notes...I think the "just use the name with strsplit" plan is probably not going to work as simply as I had hoped.

Could you add an explicit test in test-pkg-arrow.R making sure that arrow_substrait_compiler(mtcars)$resolve_function("add", 1, 2) (or something similarly simplified)` resolves the correct URI anchor? Similarly, I think an explicit test of actually evaluating a plan with a function from each URI would be appropriate (I believe there's already an explicit test for a plan with "add").

@@ -94,7 +94,7 @@ jobs:

- name: Install DuckDB
run: |
R -e "remotes::install_github("duckdb/[email protected]", subdir ='tools/rpkg', build = FALSE)"
R -e "remotes::install_github('duckdb/[email protected]', subdir ='tools/rpkg', build = FALSE)"
Copy link
Contributor

@paleolimbot paleolimbot Nov 22, 2022

Choose a reason for hiding this comment

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

Suggested change
R -e "remotes::install_github('duckdb/duckdb@0.6.0', subdir ='tools/rpkg', build = FALSE)"
R -e "remotes::install_github('cran/duckdb@0.5.0', build = FALSE)"

...maybe?

@paleolimbot
Copy link
Contributor

I just pushed one more try at getting DuckDB to work in CI. I get a clean CMD check from my end, but I do see:

Warning: [compiler.R:31] Undocumented R6 method: extension_uri_anchor

When I run devtools::document(). It would probably be a good idea to document that method.

@paleolimbot
Copy link
Contributor

If that duckdb hack doesn't work, open a ticket and I'll take a look. It would also be nice to cache the Arrow build so that it doesn't have to rebuild for every commit and both of those could be the same ticket (or maybe DuckDB's release of 0.6.0 will solve that problem entirely).

@thisisnic
Copy link
Contributor Author

I've opened #211 and skipped those tests running if DuckDB isn't installed with Substrait.

@thisisnic thisisnic mentioned this pull request Nov 24, 2022
@thisisnic thisisnic merged commit f9dfd77 into voltrondata:main Nov 24, 2022
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.

Add additional extension URI to Arrow Substrait Compiler
2 participants