Proper bindings to rust library in R package#281
Merged
thomasp85 merged 19 commits intoposit-dev:mainfrom Apr 17, 2026
Merged
Conversation
|
I recommend referring to the sedonadb package to build inside monorepo. |
Collaborator
Author
|
I'm afraid I'm not entirely sure what the relevance to this PR is? |
|
Oh, sorry. I just wanted to point out that, as you can see from the current CI failure in this type of monorepo, Rust-based R packages cannot pass R CMD checks without some workarounds. |
Collaborator
Author
|
Ah, gotcha. Thanks for the suggestion. I'm not worrying about CI with this PR just yet🙂 |
georgestagg
approved these changes
Apr 17, 2026
Collaborator
georgestagg
left a comment
georgestagg
left a comment
There was a problem hiding this comment.
Other than what looks like a minor merge reversal, LGTM!
Collaborator
There was a problem hiding this comment.
We should add this to https://github.com/rstudio/hex-stickers
Comment on lines
+179
to
+181
| // not if the columns are valid. We skip this check for wildcards | ||
| // (either layer or global). | ||
| let is_annotation = matches!( | ||
| layer.source, | ||
| Some(crate::plot::types::DataSource::Annotation) | ||
| ); | ||
| let has_wildcard = | ||
| layer.mappings.wildcard || (!is_annotation && plot.global_mappings.wildcard); | ||
| if !has_wildcard { | ||
| // Merge global mappings into a temporary copy for validation | ||
| // (mirrors execution-time merge, layer takes precedence) | ||
| let mut merged = layer.clone(); | ||
| if !is_annotation { | ||
| for (aesthetic, value) in &plot.global_mappings.aesthetics { | ||
| merged | ||
| .mappings | ||
| .aesthetics | ||
| .entry(aesthetic.clone()) | ||
| .or_insert(value.clone()); | ||
| } | ||
| } | ||
| if let Err(e) = merged.validate_mapping(&plot.aesthetic_context, false) { | ||
| // not if the columns are valid. We skip this check for wildcards. | ||
| if !layer.mappings.wildcard { | ||
| if let Err(e) = layer.validate_mapping(&plot.aesthetic_context, false) { |
Collaborator
There was a problem hiding this comment.
Was this intentional? It looks like a merge from main got reversed?
Collaborator
Author
There was a problem hiding this comment.
ugh, that is some merge conflict gone wrong because I also had a fix for the thing you fixed in main
Good catch
Comment on lines
-179
to
-358
| // not if the columns are valid. We skip this check for wildcards | ||
| // (either layer or global). | ||
| let is_annotation = matches!( | ||
| layer.source, | ||
| Some(crate::plot::types::DataSource::Annotation) | ||
| ); | ||
| let has_wildcard = | ||
| layer.mappings.wildcard || (!is_annotation && plot.global_mappings.wildcard); | ||
| if !has_wildcard { | ||
| // Merge global mappings into a temporary copy for validation | ||
| // (mirrors execution-time merge, layer takes precedence) | ||
| let mut merged = layer.clone(); | ||
| if !is_annotation { | ||
| for (aesthetic, value) in &plot.global_mappings.aesthetics { | ||
| merged | ||
| .mappings | ||
| .aesthetics | ||
| .entry(aesthetic.clone()) | ||
| .or_insert(value.clone()); | ||
| } | ||
| } | ||
| if let Err(e) = merged.validate_mapping(&plot.aesthetic_context, false) { | ||
| // not if the columns are valid. We skip this check for wildcards. | ||
| if !layer.mappings.wildcard { | ||
| if let Err(e) = layer.validate_mapping(&plot.aesthetic_context, false) { | ||
| errors.push(ValidationError { | ||
| message: format!("{}: {}", context, e), | ||
| location: None, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| // Validate SETTING parameters | ||
| if let Err(e) = layer.validate_settings() { | ||
| errors.push(ValidationError { | ||
| message: format!("{}: {}", context, e), | ||
| location: None, | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(Validated { | ||
| sql: sql_part, | ||
| visual: viz_part, | ||
| has_visual, | ||
| tree: Some(source_tree.tree), | ||
| valid: errors.is_empty(), | ||
| errors, | ||
| warnings, | ||
| }) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_validate_with_visual() { | ||
| let validated = | ||
| validate("SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x, y AS y").unwrap(); | ||
| assert!(validated.has_visual()); | ||
| assert_eq!(validated.sql(), "SELECT 1 as x, 2 as y"); | ||
| assert!(validated.visual().starts_with("VISUALISE")); | ||
| assert!(validated.tree().is_some()); | ||
| assert!(validated.valid()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_without_visual() { | ||
| let validated = validate("SELECT 1 as x, 2 as y").unwrap(); | ||
| assert!(!validated.has_visual()); | ||
| assert_eq!(validated.sql(), "SELECT 1 as x, 2 as y"); | ||
| assert!(validated.visual().is_empty()); | ||
| assert!(validated.tree().is_none()); | ||
| assert!(validated.valid()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_valid_query() { | ||
| let validated = | ||
| validate("SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x, y AS y").unwrap(); | ||
| assert!( | ||
| validated.valid(), | ||
| "Expected valid query: {:?}", | ||
| validated.errors() | ||
| ); | ||
| assert!(validated.errors().is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_missing_required_aesthetic() { | ||
| // Point requires x and y, but we only provide x | ||
| let validated = | ||
| validate("SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x").unwrap(); | ||
| assert!(!validated.valid()); | ||
| assert!(!validated.errors().is_empty()); | ||
| assert!(validated.errors()[0].message.contains("y")); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_syntax_error() { | ||
| let validated = validate("SELECT 1 VISUALISE DRAW invalidgeom").unwrap(); | ||
| assert!(!validated.valid()); | ||
| assert!(!validated.errors().is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_sql_and_visual_content() { | ||
| let query = "SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x, y AS y DRAW line MAPPING x AS x, y AS y"; | ||
| let validated = validate(query).unwrap(); | ||
|
|
||
| assert!(validated.has_visual()); | ||
| assert_eq!(validated.sql(), "SELECT 1 as x, 2 as y"); | ||
| assert!(validated.visual().contains("DRAW point")); | ||
| assert!(validated.visual().contains("DRAW line")); | ||
| assert!(validated.valid()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_sql_only() { | ||
| let query = "SELECT 1 as x, 2 as y"; | ||
| let validated = validate(query).unwrap(); | ||
|
|
||
| // SQL-only queries should be valid (just syntax check) | ||
| assert!(validated.valid()); | ||
| assert!(validated.errors().is_empty()); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_color_aesthetic_on_line() { | ||
| // color should be valid on line geom (has stroke) | ||
| let validated = validate( | ||
| "SELECT 1 as x, 2 as y VISUALISE DRAW line MAPPING x AS x, y AS y, region AS color", | ||
| ) | ||
| .unwrap(); | ||
| assert!( | ||
| validated.valid(), | ||
| "color should be accepted on line geom: {:?}", | ||
| validated.errors() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_color_aesthetic_on_point() { | ||
| // color should be valid on point geom (has stroke + fill) | ||
| let validated = validate( | ||
| "SELECT 1 as x, 2 as y VISUALISE DRAW point MAPPING x AS x, y AS y, cat AS color", | ||
| ) | ||
| .unwrap(); | ||
| assert!( | ||
| validated.valid(), | ||
| "color should be accepted on point geom: {:?}", | ||
| validated.errors() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_colour_spelling() { | ||
| // British spelling 'colour' should work (normalized by parser to 'color') | ||
| let validated = validate( | ||
| "SELECT 1 as x, 2 as y VISUALISE DRAW line MAPPING x AS x, y AS y, region AS colour", | ||
| ) | ||
| .unwrap(); | ||
| assert!( | ||
| validated.valid(), | ||
| "colour (British) should be accepted: {:?}", | ||
| validated.errors() | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_validate_global_color_mapping() { | ||
| // Global color mapping should validate correctly | ||
| let validated = | ||
| validate("SELECT 1 as x, 2 as y VISUALISE x AS x, y AS y, region AS color DRAW line") | ||
| .unwrap(); | ||
| assert!( | ||
| validated.valid(), | ||
| "global color mapping should be accepted: {:?}", | ||
| validated.errors() | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
Again, should this be left in place?
Collaborator
|
Ah, wait, we seem to have lost plots height in docs: 
|
eitsupi
reviewed
Apr 17, 2026
Comment on lines
+5
to
+16
| df_to_ipc <- function(df) { | ||
| # Convert factors to character — nanoarrow doesn't support dictionary IPC encoding | ||
| factor_cols <- vapply(df, is.factor, logical(1)) | ||
| if (any(factor_cols)) { | ||
| df[factor_cols] <- lapply(df[factor_cols], as.character) | ||
| } | ||
| stream <- nanoarrow::as_nanoarrow_array_stream(df) | ||
| con <- rawConnection(raw(0), "wb") | ||
| on.exit(close(con)) | ||
| nanoarrow::write_nanoarrow(stream, con) | ||
| rawConnectionValue(con) | ||
| } |
There was a problem hiding this comment.
Is there any reason not to implement zero-copying using Arrow C streams?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #162