Skip to content

Commit 1e4de85

Browse files
teunbrandclaude
andcommitted
Simplify line segmentation using partition_by for discrete/continuous detection
Key changes: - Use layer.partition_by to distinguish discrete vs continuous material aesthetics - Discrete aesthetics (linetype, discrete stroke/linewidth) already in partition_by from execute phase - define groups naturally, no action needed - Continuous aesthetics (numeric stroke/linewidth) checked for within-group variation and trigger segmentation if they vary, but never added to partition columns - Remove linetype from material aesthetics check (always discrete, already handled) - Remove LineSegmentMetadata struct - use layer.partition_by directly in finalize() This leverages existing information computed during execute phase rather than re-inferring or passing through metadata. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent b5d2278 commit 1e4de85

1 file changed

Lines changed: 28 additions & 57 deletions

File tree

src/writer/vegalite/layer.rs

Lines changed: 28 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -420,16 +420,12 @@ fn aesthetic_varies_within_groups(
420420
Ok(false)
421421
}
422422

423-
/// Metadata for segmented line rendering
424-
#[derive(Debug)]
425-
struct LineSegmentMetadata {
426-
partition_columns: Vec<String>,
427-
}
428-
429423
/// Renderer for line geom - preserves data order for correct line rendering
430424
///
431-
/// Automatically detects when material aesthetics (stroke, linetype) vary within
432-
/// partition groups and converts to segmented rendering using detail encoding.
425+
/// Automatically detects when continuous material aesthetics (stroke, linewidth) vary
426+
/// within partition groups and converts to segmented rendering using detail encoding.
427+
/// Discrete material aesthetics (linetype, or discrete stroke) already define groups
428+
/// via partition_by and don't require special handling.
433429
pub struct LineRenderer;
434430

435431
impl GeomRenderer for LineRenderer {
@@ -440,32 +436,14 @@ impl GeomRenderer for LineRenderer {
440436
_data_key: &str,
441437
binned_columns: &HashMap<String, Vec<f64>>,
442438
) -> Result<PreparedData> {
443-
// Identify material aesthetics that are column-mapped
444-
let material_aesthetics = ["stroke", "linetype"];
445-
let mut varying_aesthetics = Vec::new();
439+
// Continuous material aesthetics that can trigger segmentation
440+
// (linetype is always discrete and already handled via partition_by)
441+
let material_aesthetics = ["stroke", "linewidth"];
446442

447-
// Collect (aesthetic, column) pairs for material aesthetics that are mapped to columns
448-
let mapped_material_aesthetics: Vec<(&str, String)> = material_aesthetics
449-
.iter()
450-
.filter_map(|aesthetic| {
451-
if let Some(AestheticValue::Column { name: col, .. }) = layer.mappings.get(aesthetic) {
452-
Some((*aesthetic, col.clone()))
453-
} else {
454-
None
455-
}
456-
})
457-
.collect();
458-
459-
// Build list of partition columns EXCLUDING the material aesthetics we're checking
460-
// (we need to check if they vary within the other partition groups)
461-
let mut partition_columns: Vec<String> = layer
462-
.partition_by
463-
.iter()
464-
.filter(|col| !mapped_material_aesthetics.iter().any(|(_, c)| c == *col))
465-
.cloned()
466-
.collect();
443+
// Start with existing partition_by (includes discrete material aesthetics already)
444+
let partition_columns: Vec<String> = layer.partition_by.clone();
467445

468-
// Compute group boundaries once (without the material aesthetics)
446+
// Compute group boundaries based on existing partitions
469447
let n_rows = df.height();
470448
let group_boundaries = if partition_columns.is_empty() || n_rows <= 1 {
471449
vec![0, n_rows]
@@ -475,16 +453,19 @@ impl GeomRenderer for LineRenderer {
475453
boundaries
476454
};
477455

478-
// Check each mapped material aesthetic for within-group variation
479-
for (aesthetic, col) in &mapped_material_aesthetics {
480-
// Check if this aesthetic varies within partition groups
481-
let varies = aesthetic_varies_within_groups(df, col, &group_boundaries)?;
482-
if varies {
483-
varying_aesthetics.push(*aesthetic);
484-
} else {
485-
// If it doesn't vary within groups, treat it as a partition column
486-
// (boundaries remain the same since the aesthetic changes only where partitions change)
487-
partition_columns.push(col.clone());
456+
// Check continuous material aesthetics (not in partition_by) for within-group variation
457+
let mut varying_aesthetics: Vec<&str> = Vec::new();
458+
459+
for aesthetic in material_aesthetics {
460+
if let Some(AestheticValue::Column { name: col, .. }) = layer.mappings.get(aesthetic) {
461+
// Skip if already in partition_by (discrete, already defines groups)
462+
if !layer.partition_by.contains(col) {
463+
// Continuous: check if varies within groups
464+
if aesthetic_varies_within_groups(df, col, &group_boundaries)? {
465+
varying_aesthetics.push(aesthetic);
466+
// Don't add to partition_columns - continuous values shouldn't partition
467+
}
468+
}
488469
}
489470
}
490471

@@ -502,7 +483,7 @@ impl GeomRenderer for LineRenderer {
502483
// This ensures the source filter works correctly with the unified dataset
503484
Ok(PreparedData::Composite {
504485
components: [("".to_string(), values)].iter().cloned().collect(),
505-
metadata: Box::new(LineSegmentMetadata { partition_columns }),
486+
metadata: Box::new(()),
506487
})
507488
} else {
508489
Ok(PreparedData::Single { values })
@@ -527,25 +508,15 @@ impl GeomRenderer for LineRenderer {
527508
fn finalize(
528509
&self,
529510
mut layer_spec: Value,
530-
_layer: &Layer,
511+
layer: &Layer,
531512
_data_key: &str,
532513
prepared: &PreparedData,
533514
) -> Result<Vec<Value>> {
534515
// Early return for standard line rendering
535-
let PreparedData::Composite { metadata, .. } = prepared else {
516+
let PreparedData::Composite { .. } = prepared else {
536517
return Ok(vec![layer_spec]);
537518
};
538519

539-
// Extract partition columns from metadata
540-
let metadata_any = metadata.as_ref() as &dyn Any;
541-
let partition_columns = if let Some(meta) = metadata_any.downcast_ref::<LineSegmentMetadata>() {
542-
&meta.partition_columns
543-
} else {
544-
return Err(GgsqlError::InternalError(
545-
"Invalid metadata type for segmented line".to_string(),
546-
));
547-
};
548-
549520
// Get position column names
550521
let x_col = naming::aesthetic_column("pos1");
551522
let y_col = naming::aesthetic_column("pos2");
@@ -583,8 +554,8 @@ impl GeomRenderer for LineRenderer {
583554
"sort": [{"field": ROW_INDEX_COLUMN}]
584555
});
585556

586-
if !partition_columns.is_empty() {
587-
window_transform["groupby"] = json!(partition_columns);
557+
if !layer.partition_by.is_empty() {
558+
window_transform["groupby"] = json!(layer.partition_by);
588559
}
589560

590561
transforms.push(window_transform);

0 commit comments

Comments
 (0)