Skip to content

Commit f861c68

Browse files
thomasp85thomasp85
committed
simplify orientation resolving
1 parent 583a420 commit f861c68

7 files changed

Lines changed: 103 additions & 100 deletions

File tree

‎src/execute/layer.rs‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
66
use crate::plot::aesthetic::{is_positional_aesthetic, AestheticContext};
77
use crate::plot::layer::is_transposed;
8+
use crate::plot::layer::orientation::{flip_mappings, resolve_orientation};
89
use crate::plot::{
910
AestheticValue, DefaultAestheticValue, Layer, ParameterValue, Scale, Schema, SqlTypeNames,
1011
StatResult,
@@ -362,7 +363,7 @@ where
362363

363364
// Orientation detection and initial flip was already done in mod.rs before
364365
// build_layer_base_query. We just check if we need to flip back after stat.
365-
let needs_flip = is_transposed(layer, scales);
366+
let needs_flip = is_transposed(layer);
366367

367368
// Build the aesthetic-named schema for stat transforms
368369
// Note: Mappings were already flipped in mod.rs if needed, so schema reflects normalized orientation
@@ -681,3 +682,46 @@ pub fn flip_dataframe_positional_columns(
681682

682683
lazy.collect().expect("rename should not fail")
683684
}
685+
686+
/// Resolve orientation for all layers and apply mapping flips.
687+
///
688+
/// This function:
689+
/// 1. Resolves orientation via auto-detection or explicit setting
690+
/// 2. Stores resolved orientation in layer parameters
691+
/// 3. Flips mappings for transposed layers
692+
/// 4. Flips type_info column names to match flipped mappings
693+
///
694+
/// Must be called BEFORE building base queries, since build_layer_base_query
695+
/// uses layer.mappings to create SQL like `column AS __ggsql_aes_pos1__`.
696+
///
697+
/// Note: Validation of orientation settings is handled by `validate_settings()`,
698+
/// which rejects orientation for geoms that don't have it in default_params.
699+
pub fn resolve_orientations(
700+
layers: &mut [Layer],
701+
scales: &[Scale],
702+
layer_type_info: &mut [Vec<super::schema::TypeInfo>],
703+
aesthetic_ctx: &AestheticContext,
704+
) {
705+
for (layer_idx, layer) in layers.iter_mut().enumerate() {
706+
let orientation = resolve_orientation(layer, scales);
707+
// Store resolved orientation in parameters for downstream use (writers need it)
708+
layer.parameters.insert(
709+
"orientation".to_string(),
710+
ParameterValue::String(orientation.to_string()),
711+
);
712+
if is_transposed(layer) {
713+
flip_mappings(layer);
714+
// Also flip column names in type_info to match the flipped mappings
715+
if layer_idx < layer_type_info.len() {
716+
for (name, _, _) in &mut layer_type_info[layer_idx] {
717+
if let Some(aesthetic) = naming::extract_aesthetic_name(name) {
718+
let flipped = aesthetic_ctx.flip_positional(aesthetic);
719+
if flipped != aesthetic {
720+
*name = naming::aesthetic_column(&flipped);
721+
}
722+
}
723+
}
724+
}
725+
}
726+
}
727+
}

‎src/execute/mod.rs‎

Lines changed: 11 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1027,69 +1027,14 @@ pub fn prepare_data_with_reader<R: Reader>(query: &str, reader: &R) -> Result<Pr
10271027

10281028
// Detect orientation and flip mappings BEFORE building base queries.
10291029
// This ensures the SQL query uses the correct aesthetic column names.
1030-
// The flip must happen here (not in apply_layer_transforms) because
1031-
// build_layer_base_query uses layer.mappings to create SQL like
1032-
// `column AS __ggsql_aes_pos1__`.
1033-
{
1034-
use crate::plot::layer::orientation::{
1035-
flip_mappings, geom_has_implicit_orientation, resolve_orientation,
1036-
};
1037-
use crate::plot::ParameterValue;
1038-
let scales = specs[0].scales.clone();
1039-
let aesthetic_ctx = specs[0].get_aesthetic_context();
1040-
for (layer_idx, layer) in specs[0].layers.iter_mut().enumerate() {
1041-
// Check if user tried to set orientation
1042-
let geom_type = layer.geom.geom_type();
1043-
if layer.parameters.contains_key("orientation") {
1044-
// Geoms with implicit orientation (bar, histogram, etc.): reject, use auto-detection
1045-
if geom_has_implicit_orientation(&geom_type) {
1046-
return Err(GgsqlError::ValidationError(format!(
1047-
"Layer {}: orientation is auto-detected for {} geom and cannot be set via SETTING. \
1048-
Use aesthetic mappings to control orientation (e.g., MAPPING category AS y for horizontal bars).",
1049-
layer_idx + 1,
1050-
geom_type
1051-
)));
1052-
}
1053-
1054-
// Geoms without orientation in default_params (point, path, polygon): reject
1055-
let has_orientation_default = layer
1056-
.geom
1057-
.default_params()
1058-
.iter()
1059-
.any(|p| p.name == "orientation");
1060-
if !has_orientation_default {
1061-
return Err(GgsqlError::ValidationError(format!(
1062-
"Layer {}: Invalid setting 'orientation' for geom '{}'. \
1063-
This geom does not support orientation.",
1064-
layer_idx + 1,
1065-
geom_type
1066-
)));
1067-
}
1068-
// Otherwise (line, linear, area): allow user-provided orientation
1069-
}
1070-
1071-
let orientation = resolve_orientation(layer, &scales);
1072-
// Store resolved orientation in parameters for downstream use (writers need it)
1073-
layer.parameters.insert(
1074-
"orientation".to_string(),
1075-
ParameterValue::String(orientation.to_string()),
1076-
);
1077-
if is_transposed(layer, &scales) {
1078-
flip_mappings(layer);
1079-
// Also flip column names in type_info to match the flipped mappings
1080-
if layer_idx < layer_type_info.len() {
1081-
for (name, _, _) in &mut layer_type_info[layer_idx] {
1082-
if let Some(aesthetic) = naming::extract_aesthetic_name(name) {
1083-
let flipped = aesthetic_ctx.flip_positional(aesthetic);
1084-
if flipped != aesthetic {
1085-
*name = naming::aesthetic_column(&flipped);
1086-
}
1087-
}
1088-
}
1089-
}
1090-
}
1091-
}
1092-
}
1030+
let scales = specs[0].scales.clone();
1031+
let aesthetic_ctx = specs[0].get_aesthetic_context();
1032+
layer::resolve_orientations(
1033+
&mut specs[0].layers,
1034+
&scales,
1035+
&mut layer_type_info,
1036+
&aesthetic_ctx,
1037+
);
10931038

10941039
// Build layer base queries using build_layer_base_query()
10951040
// These include: SELECT with aesthetic renames, casts from type_requirements, filters
@@ -1179,7 +1124,7 @@ pub fn prepare_data_with_reader<R: Reader>(query: &str, reader: &R) -> Result<Pr
11791124
for (idx, q) in layer_queries.iter().enumerate() {
11801125
let layer = &mut specs[0].layers[idx];
11811126
let remappings_key = serde_json::to_string(&layer.remappings).unwrap_or_default();
1182-
let needs_flip = is_transposed(layer, &scales);
1127+
let needs_flip = is_transposed(layer);
11831128
let config_key = (q.clone(), remappings_key, needs_flip);
11841129

11851130
if let Some(existing_key) = config_to_key.get(&config_key) {
@@ -1219,7 +1164,7 @@ pub fn prepare_data_with_reader<R: Reader>(query: &str, reader: &R) -> Result<Pr
12191164
// with ALIGNED orientation names) but BEFORE update_mappings_for_remappings
12201165
// (which uses remapping keys to create mapping entries).
12211166
// Phase 4.5 will then flip the DataFrame columns to match.
1222-
if is_transposed(l, &scales) {
1167+
if is_transposed(l) {
12231168
crate::plot::layer::orientation::flip_remappings(l);
12241169
}
12251170

@@ -1239,7 +1184,7 @@ pub fn prepare_data_with_reader<R: Reader>(query: &str, reader: &R) -> Result<Pr
12391184
// All positional columns (stat-produced and literal) are flipped together.
12401185
let mut flipped_keys: HashSet<String> = HashSet::new();
12411186
for layer in specs[0].layers.iter() {
1242-
if is_transposed(layer, &scales) {
1187+
if is_transposed(layer) {
12431188
if let Some(ref key) = layer.data_key {
12441189
if flipped_keys.insert(key.clone()) {
12451190
// First time flipping this data key

‎src/lib.rs‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -867,7 +867,8 @@ mod integration_tests {
867867
// Test orientation setting behavior for different geom types
868868
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
869869

870-
// 1. Bar geom (has implicit orientation) - should reject with auto-detection message
870+
// 1. Bar geom (has implicit orientation) - should reject
871+
// Orientation is auto-detected based on mappings, not settable via SETTING
871872
let query = r#"
872873
SELECT 'A' as category, 10 as value
873874
VISUALISE
@@ -879,8 +880,8 @@ mod integration_tests {
879880
Err(e) => {
880881
let err = e.to_string();
881882
assert!(
882-
err.contains("orientation is auto-detected"),
883-
"Error should mention auto-detection: {}",
883+
err.contains("Invalid setting 'orientation'"),
884+
"Error should mention invalid setting: {}",
884885
err
885886
);
886887
assert!(
@@ -904,8 +905,8 @@ mod integration_tests {
904905
Err(e) => {
905906
let err2 = e.to_string();
906907
assert!(
907-
err2.contains("does not support orientation"),
908-
"Error should mention lack of support: {}",
908+
err2.contains("Invalid setting 'orientation'"),
909+
"Error should mention invalid setting: {}",
909910
err2
910911
);
911912
}

‎src/plot/layer/geom/mod.rs‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -220,15 +220,12 @@ pub trait GeomTrait: std::fmt::Debug + std::fmt::Display + Send + Sync {
220220

221221
/// Returns valid parameter names for SETTING clause.
222222
///
223-
/// Combines supported aesthetics with non-aesthetic parameters.
224-
/// Includes "orientation" which is set internally during execution (not user-settable).
223+
/// Combines supported aesthetics with non-aesthetic parameters from default_params.
225224
fn valid_settings(&self) -> Vec<&'static str> {
226225
let mut valid: Vec<&'static str> = self.aesthetics().supported();
227226
for param in self.default_params() {
228227
valid.push(param.name);
229228
}
230-
// Internal parameter set during execution (not user-settable via SETTING)
231-
valid.push("orientation");
232229
valid
233230
}
234231
}

‎src/plot/layer/orientation.rs‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,9 +64,15 @@ pub fn resolve_orientation(layer: &Layer, scales: &[Scale]) -> &'static str {
6464

6565
/// Check if a layer is transposed (horizontal orientation).
6666
///
67-
/// Convenience helper for downstream code that needs to check orientation.
68-
pub fn is_transposed(layer: &Layer, scales: &[Scale]) -> bool {
69-
resolve_orientation(layer, scales) == TRANSPOSED
67+
/// Reads the orientation from the layer's parameters, which must have been
68+
/// set by `resolve_orientations()` during execution.
69+
pub fn is_transposed(layer: &Layer) -> bool {
70+
layer
71+
.parameters
72+
.get("orientation")
73+
.and_then(|v| v.as_str())
74+
.map(|s| s == TRANSPOSED)
75+
.unwrap_or(false)
7076
}
7177

7278
/// Check if a geom type supports orientation auto-detection.
@@ -302,6 +308,8 @@ mod tests {
302308

303309
#[test]
304310
fn test_is_transposed_helper() {
311+
use crate::plot::ParameterValue;
312+
305313
// Helper function should return true for transposed orientation
306314
let mut layer = Layer::new(Geom::histogram());
307315
layer
@@ -311,15 +319,26 @@ mod tests {
311319
scale.scale_type = Some(ScaleType::continuous());
312320
let scales = vec![scale];
313321

314-
assert!(is_transposed(&layer, &scales));
322+
// Resolve and store orientation (as done by resolve_orientations)
323+
let orientation = resolve_orientation(&layer, &scales);
324+
layer.parameters.insert(
325+
"orientation".to_string(),
326+
ParameterValue::String(orientation.to_string()),
327+
);
328+
assert!(is_transposed(&layer));
315329

316330
// Should return false for aligned orientation
317-
let layer2 = Layer::new(Geom::histogram());
331+
let mut layer2 = Layer::new(Geom::histogram());
318332
let mut scale2 = Scale::new("pos1");
319333
scale2.scale_type = Some(ScaleType::continuous());
320334
let scales2 = vec![scale2];
321335

322-
assert!(!is_transposed(&layer2, &scales2));
336+
let orientation2 = resolve_orientation(&layer2, &scales2);
337+
layer2.parameters.insert(
338+
"orientation".to_string(),
339+
ParameterValue::String(orientation2.to_string()),
340+
);
341+
assert!(!is_transposed(&layer2));
323342
}
324343

325344
#[test]

‎src/writer/vegalite/layer.rs‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -271,15 +271,15 @@ impl GeomRenderer for BarRenderer {
271271
&self,
272272
layer_spec: &mut Value,
273273
layer: &Layer,
274-
context: &RenderContext,
274+
_context: &RenderContext,
275275
) -> Result<()> {
276276
let width = match layer.parameters.get("width") {
277277
Some(ParameterValue::Number(w)) => *w,
278278
_ => 0.9,
279279
};
280280

281281
// For horizontal bars, use "height" for band size; for vertical, use "width"
282-
let is_horizontal = is_transposed(layer, context.scales);
282+
let is_horizontal = is_transposed(layer);
283283

284284
// For dodged bars, use expression-based size with the adjusted width
285285
// For non-dodged bars, use band-relative size
@@ -447,14 +447,14 @@ impl GeomRenderer for LinearRenderer {
447447
&self,
448448
encoding: &mut Map<String, Value>,
449449
layer: &Layer,
450-
context: &RenderContext,
450+
_context: &RenderContext,
451451
) -> Result<()> {
452452
// Remove coefficient and intercept from encoding - they're only used in transforms
453453
encoding.remove("coef");
454454
encoding.remove("intercept");
455455

456456
// Check orientation
457-
let is_horizontal = is_transposed(layer, context.scales);
457+
let is_horizontal = is_transposed(layer);
458458

459459
// For aligned (default): x is primary axis, y is computed (secondary)
460460
// For transposed: y is primary axis, x is computed (secondary)
@@ -508,7 +508,7 @@ impl GeomRenderer for LinearRenderer {
508508
let intercept_field = naming::aesthetic_column("intercept");
509509

510510
// Check orientation
511-
let is_horizontal = is_transposed(layer, context.scales);
511+
let is_horizontal = is_transposed(layer);
512512

513513
// Get extent from appropriate axis:
514514
// - Aligned (default): extent from pos1 (x-axis), compute y from x
@@ -565,9 +565,9 @@ impl GeomRenderer for RibbonRenderer {
565565
&self,
566566
encoding: &mut Map<String, Value>,
567567
layer: &Layer,
568-
context: &RenderContext,
568+
_context: &RenderContext,
569569
) -> Result<()> {
570-
let is_horizontal = is_transposed(layer, context.scales);
570+
let is_horizontal = is_transposed(layer);
571571

572572
// Remap min/max to primary/secondary based on orientation:
573573
// - Aligned (vertical): ymax→y, ymin→y2
@@ -644,7 +644,7 @@ impl GeomRenderer for ViolinRenderer {
644644
&self,
645645
layer_spec: &mut Value,
646646
layer: &Layer,
647-
context: &RenderContext,
647+
_context: &RenderContext,
648648
) -> Result<()> {
649649
layer_spec["mark"] = json!({
650650
"type": "line",
@@ -656,7 +656,7 @@ impl GeomRenderer for ViolinRenderer {
656656
let violin_offset = format!("[datum.{offset}, -datum.{offset}]", offset = offset_col);
657657

658658
// Read orientation from layer (already resolved during execution)
659-
let is_horizontal = is_transposed(layer, context.scales);
659+
let is_horizontal = is_transposed(layer);
660660

661661
// Continuous axis column for order calculation:
662662
// - Vertical: pos2 (y-axis has continuous density values)
@@ -728,10 +728,10 @@ impl GeomRenderer for ViolinRenderer {
728728
&self,
729729
encoding: &mut Map<String, Value>,
730730
layer: &Layer,
731-
context: &RenderContext,
731+
_context: &RenderContext,
732732
) -> Result<()> {
733733
// Read orientation from layer (already resolved during execution)
734-
let is_horizontal = is_transposed(layer, context.scales);
734+
let is_horizontal = is_transposed(layer);
735735

736736
// Categorical axis for detail encoding:
737737
// - Vertical: x channel (categorical groups on x-axis)
@@ -1008,12 +1008,12 @@ impl BoxplotRenderer {
10081008
layer: &Layer,
10091009
base_key: &str,
10101010
has_outliers: bool,
1011-
context: &RenderContext,
1011+
_context: &RenderContext,
10121012
) -> Result<Vec<Value>> {
10131013
let mut layers: Vec<Value> = Vec::new();
10141014

10151015
// Read orientation from layer (already resolved during execution)
1016-
let is_horizontal = is_transposed(layer, context.scales);
1016+
let is_horizontal = is_transposed(layer);
10171017

10181018
// Value columns depend on orientation (after DataFrame column flip):
10191019
// - Vertical: values in pos2/pos2end (no flip)

‎src/writer/vegalite/mod.rs‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,11 +1139,8 @@ impl Writer for VegaLiteWriter {
11391139
layer.validate_required_aesthetics().map_err(|e| {
11401140
GgsqlError::ValidationError(format!("Layer validation failed: {}", e))
11411141
})?;
1142-
1143-
// Check SETTING parameters are valid for this geom
1144-
layer.validate_settings().map_err(|e| {
1145-
GgsqlError::ValidationError(format!("Layer validation failed: {}", e))
1146-
})?;
1142+
// Note: validate_settings() is already called during execution, before
1143+
// internal parameters like "orientation" are set, so we don't call it here.
11471144
}
11481145

11491146
Ok(())

0 commit comments

Comments
 (0)