Skip to content

Commit 6b88e52

Browse files
authored
Fix literal mappings (#132)
1 parent 60fdab5 commit 6b88e52

3 files changed

Lines changed: 95 additions & 1 deletion

File tree

src/execute/layer.rs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,22 @@ where
372372
// Build the aesthetic-named schema for stat transforms
373373
let aesthetic_schema: Schema = build_aesthetic_schema(layer, schema);
374374

375+
// Collect literal aesthetic column names BEFORE conversion to Column values.
376+
// Literal columns contain constant values (same for every row), so adding them to
377+
// GROUP BY doesn't affect aggregation results - they're simply preserved through grouping.
378+
let literal_columns: Vec<String> = layer
379+
.mappings
380+
.aesthetics
381+
.iter()
382+
.filter_map(|(aesthetic, value)| {
383+
if value.is_literal() {
384+
Some(naming::aesthetic_column(aesthetic))
385+
} else {
386+
None
387+
}
388+
})
389+
.collect();
390+
375391
// Update mappings to use prefixed aesthetic names
376392
// This must happen BEFORE stat transforms so they use aesthetic names
377393
layer.update_mappings_for_aesthetic_columns();
@@ -393,6 +409,15 @@ where
393409
}
394410
}
395411

412+
// Add literal aesthetic columns to group_by so they survive stat transforms.
413+
// Since literal columns contain constant values (same for every row), adding them
414+
// to GROUP BY doesn't affect aggregation results - they're simply preserved.
415+
for col in &literal_columns {
416+
if !group_by.contains(col) {
417+
group_by.push(col.clone());
418+
}
419+
}
420+
396421
// Apply statistical transformation (uses aesthetic names)
397422
let stat_result = layer.geom.apply_stat_transform(
398423
&query,

src/execute/mod.rs

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,4 +1188,71 @@ mod tests {
11881188
assert_eq!(result.data.get(layer0_key).unwrap().height(), 3);
11891189
assert_eq!(result.data.get(layer1_key).unwrap().height(), 3);
11901190
}
1191+
1192+
/// Test that literal mappings survive stat transforms (e.g., histogram grouping).
1193+
///
1194+
/// This tests the fix for issue #129 where literal aesthetic columns like
1195+
/// `'foo' AS stroke` were lost during stat transforms because they weren't
1196+
/// included in the GROUP BY clause.
1197+
#[cfg(feature = "duckdb")]
1198+
#[test]
1199+
fn test_histogram_with_literal_mapping() {
1200+
let reader = DuckDBReader::from_connection_string("duckdb://memory").unwrap();
1201+
1202+
// Create test data
1203+
reader
1204+
.connection()
1205+
.execute(
1206+
"CREATE TABLE hist_literal_test AS SELECT RANDOM() * 100 as value FROM range(100)",
1207+
duckdb::params![],
1208+
)
1209+
.unwrap();
1210+
1211+
// Histogram with a literal stroke mapping - should preserve the literal column
1212+
let query = r#"
1213+
SELECT * FROM hist_literal_test
1214+
VISUALISE value AS x
1215+
DRAW histogram MAPPING 'foo' AS stroke
1216+
"#;
1217+
1218+
let result = prepare_data_with_reader(query, &reader).unwrap();
1219+
1220+
// Should have layer 0 data with binned results
1221+
assert!(result.data.contains_key(&naming::layer_key(0)));
1222+
let layer_df = result.data.get(&naming::layer_key(0)).unwrap();
1223+
1224+
// Should have prefixed aesthetic-named columns
1225+
let col_names: Vec<String> = layer_df
1226+
.get_column_names_str()
1227+
.iter()
1228+
.map(|s| s.to_string())
1229+
.collect();
1230+
1231+
let x_col = naming::aesthetic_column("x");
1232+
let y_col = naming::aesthetic_column("y");
1233+
let stroke_col = naming::aesthetic_column("stroke");
1234+
1235+
assert!(
1236+
col_names.contains(&x_col),
1237+
"Should have '{}' column: {:?}",
1238+
x_col,
1239+
col_names
1240+
);
1241+
assert!(
1242+
col_names.contains(&y_col),
1243+
"Should have '{}' column: {:?}",
1244+
y_col,
1245+
col_names
1246+
);
1247+
// The literal stroke column should survive the stat transform
1248+
assert!(
1249+
col_names.contains(&stroke_col),
1250+
"Should have '{}' column (literal mapping should survive stat transform): {:?}",
1251+
stroke_col,
1252+
col_names
1253+
);
1254+
1255+
// Should have fewer rows than original (binned)
1256+
assert!(layer_df.height() < 100);
1257+
}
11911258
}

src/writer/vegalite.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1303,9 +1303,11 @@ impl VegaLiteWriter {
13031303
} else {
13041304
" (global data)".to_string()
13051305
};
1306+
// Use user-friendly column name (extract aesthetic name from internal names)
1307+
let display_col = naming::extract_aesthetic_name(col).unwrap_or(col.as_str());
13061308
return Err(GgsqlError::ValidationError(format!(
13071309
"Column '{}' referenced in aesthetic '{}' (layer {}{}) does not exist.\nAvailable columns: {}",
1308-
col,
1310+
display_col,
13091311
aesthetic,
13101312
layer_idx + 1,
13111313
source_desc,

0 commit comments

Comments
 (0)