Skip to content

Commit 5f4e972

Browse files
authored
PLACE layers distinguish arrays for aesthetics vs arrays for parameters (#299)
1 parent ccdebbc commit 5f4e972

2 files changed

Lines changed: 72 additions & 2 deletions

File tree

src/execute/layer.rs

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -700,6 +700,7 @@ fn process_annotation_layer(layer: &mut Layer, dialect: &dyn SqlDialect) -> Resu
700700
// Only process position aesthetics, required aesthetics, and array parameters
701701
// (material non-required scalars stay in parameters as geom settings)
702702
let required_aesthetics = layer.geom.aesthetics().required();
703+
let supported_aesthetics = layer.geom.aesthetics().supported();
703704
let param_keys: Vec<String> = layer.parameters.keys().cloned().collect();
704705

705706
// Collect parameters we'll use, checking criteria and filtering NULLs
@@ -720,10 +721,11 @@ fn process_annotation_layer(layer: &mut Layer, dialect: &dyn SqlDialect) -> Resu
720721
continue;
721722
}
722723

723-
// Check if this is a position aesthetic OR a required aesthetic OR an array
724+
// Check if this is a position aesthetic OR a required aesthetic OR an array for supported aesthetic
724725
let is_position = crate::plot::aesthetic::is_position_aesthetic(&param_name);
725726
let is_required = required_aesthetics.contains(&param_name.as_str());
726-
let is_array = matches!(value, ParameterValue::Array(_));
727+
let is_array = matches!(value, ParameterValue::Array(_))
728+
&& supported_aesthetics.contains(&param_name.as_str());
727729

728730
// Only process position/required/array parameters
729731
if is_position || is_required || is_array {

src/execute/mod.rs

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2667,6 +2667,74 @@ mod tests {
26672667
"PLACE layer should not have stroke (text geom default is null)"
26682668
);
26692669
}
2670+
2671+
#[cfg(feature = "duckdb")]
2672+
#[test]
2673+
fn test_place_array_parameter_not_recycled() {
2674+
// Test that array parameters that are NOT supported aesthetics
2675+
// should not trigger row recycling in PLACE layers.
2676+
// Example: offset is a PARAMETER for text geom, not an aesthetic,
2677+
// so `offset => [0, 1]` should NOT create 2 rows.
2678+
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
2679+
2680+
let query = r#"
2681+
VISUALISE
2682+
PLACE text SETTING x => 5, y => 10, label => 'Test', offset => [0, 1]
2683+
"#;
2684+
2685+
let result = prepare_data_with_reader(query, &reader).unwrap();
2686+
2687+
assert_eq!(result.specs.len(), 1);
2688+
assert_eq!(
2689+
result.specs[0].layers.len(),
2690+
1,
2691+
"Should have one PLACE layer"
2692+
);
2693+
2694+
let text_layer = &result.specs[0].layers[0];
2695+
assert_eq!(text_layer.geom, crate::Geom::text());
2696+
assert!(
2697+
matches!(text_layer.source, Some(DataSource::Annotation)),
2698+
"PLACE layer should have Annotation source"
2699+
);
2700+
2701+
// Verify annotation layer has exactly 1 row (not 2)
2702+
// offset is a parameter, not an aesthetic, so it should NOT be recycled
2703+
let annotation_key = text_layer.data_key.as_ref().unwrap();
2704+
let annotation_df = result.data.get(annotation_key).unwrap();
2705+
assert_eq!(
2706+
annotation_df.height(),
2707+
1,
2708+
"Annotation layer should have exactly 1 row (offset array should not be recycled)"
2709+
);
2710+
2711+
// Verify offset remains as a parameter (not moved to aesthetics)
2712+
assert!(
2713+
text_layer.parameters.contains_key("offset"),
2714+
"offset should remain as a parameter"
2715+
);
2716+
assert!(
2717+
!text_layer.mappings.contains_key("offset"),
2718+
"offset should NOT be moved to aesthetics/mappings"
2719+
);
2720+
2721+
// Verify offset has the original array value
2722+
match text_layer.parameters.get("offset") {
2723+
Some(crate::plot::types::ParameterValue::Array(arr)) => {
2724+
assert_eq!(arr.len(), 2, "offset should have 2 elements");
2725+
assert!(
2726+
matches!(arr[0], crate::plot::types::ArrayElement::Number(n) if (n - 0.0).abs() < 1e-10),
2727+
"offset[0] should be 0"
2728+
);
2729+
assert!(
2730+
matches!(arr[1], crate::plot::types::ArrayElement::Number(n) if (n - 1.0).abs() < 1e-10),
2731+
"offset[1] should be 1"
2732+
);
2733+
}
2734+
other => panic!("Expected offset to be Array, got: {:?}", other),
2735+
}
2736+
}
2737+
26702738
#[cfg(feature = "duckdb")]
26712739
#[test]
26722740
fn test_null_mapping_removes_global_aesthetic() {

0 commit comments

Comments
 (0)