Update sum([null]) aggregation to be null to match Vega 5.26.2 (#441)

* Update sum([null]) aggregation to be null to match Vega 5.26.2 * Comment test until Vega 5.26.2 update
vega · Jan 2, 2024 · f3ac3ba · f3ac3ba
1 parent 93be376
commit f3ac3ba
Show file tree

Hide file tree

Showing 4 changed files with 9 additions and 16 deletions.
diff --git a/vegafusion-runtime/src/transform/aggregate.rs b/vegafusion-runtime/src/transform/aggregate.rs
@@ -1,9 +1,7 @@
 use crate::expression::compiler::config::CompilationConfig;
 use crate::transform::TransformTrait;
 
-use datafusion_expr::{
-    avg, count, count_distinct, lit, max, min, sum, BuiltinScalarFunction, Expr, ExprSchemable,
-};
+use datafusion_expr::{avg, count, count_distinct, lit, max, min, sum, Expr};
 use std::collections::HashMap;
 
 use async_trait::async_trait;
@@ -12,7 +10,7 @@ use std::sync::Arc;
 use vegafusion_common::column::{flat_col, unescaped_col};
 use vegafusion_common::data::ORDER_COL;
 use vegafusion_common::datafusion_common::{DFSchema, ScalarValue};
-use vegafusion_common::datatypes::{cast_to, to_numeric};
+use vegafusion_common::datatypes::to_numeric;
 use vegafusion_common::error::ResultWithContext;
 use vegafusion_common::escape::unescape_field;
 use vegafusion_core::arrow::datatypes::DataType;
@@ -162,15 +160,7 @@ pub fn make_agg_expr_for_col_expr(
         AggregateOp::Mean | AggregateOp::Average => avg(numeric_column()?),
         AggregateOp::Min => min(column),
         AggregateOp::Max => max(column),
-        AggregateOp::Sum => {
-            let numeric_col = numeric_column()?;
-            let dtype = numeric_col.get_type(schema)?;
-            let zero = cast_to(lit(0), &dtype, schema)?;
-            Expr::ScalarFunction(expr::ScalarFunction {
-                fun: BuiltinScalarFunction::Coalesce,
-                args: vec![sum(numeric_col), zero],
-            })
-        }
+        AggregateOp::Sum => sum(numeric_column()?),
         AggregateOp::Median => Expr::AggregateFunction(expr::AggregateFunction {
             fun: aggregate_function::AggregateFunction::Median,
             distinct: false,

diff --git a/vegafusion-runtime/tests/test_image_comparison.rs b/vegafusion-runtime/tests/test_image_comparison.rs
@@ -144,7 +144,8 @@ mod test_custom_specs {
         case("custom/gh_391", 0.001, true),
         case("custom/facet_grouped_bar_with_error_bars", 0.001, true),
         case("custom/facet_grouped_bar_with_error_bars_with_sort", 0.001, true),
-        case("custom/binned_ordinal", 0.001, true),
+        // Re-enable after updating to Vega 5.26.2
+        // case("custom/binned_ordinal", 0.001, true),
         case("custom/timeOffset_stocks", 0.001, true),
         case("custom/quakes_initial_selection", 0.001, true),
         case("custom/aggregate_with_threshold", 0.001, true),

diff --git a/vegafusion-runtime/tests/test_transform_aggregate.rs b/vegafusion-runtime/tests/test_transform_aggregate.rs
@@ -337,7 +337,8 @@ mod test_aggregate_nulls {
         case(AggregateOpSpec::Valid),
         case(AggregateOpSpec::Missing),
         case(AggregateOpSpec::Distinct),
-        case(AggregateOpSpec::Sum),
+        // Re-enable after updating to Vega 5.26.2 when sum([null]) behavior will matche
+        // case(AggregateOpSpec::Sum),
         case(AggregateOpSpec::Mean),
         case(AggregateOpSpec::Average),
         case(AggregateOpSpec::Min),

diff --git a/vegafusion-runtime/tests/test_transform_joinaggregate.rs b/vegafusion-runtime/tests/test_transform_joinaggregate.rs
@@ -205,7 +205,8 @@ mod test_aggregate_nulls {
         case(AggregateOpSpec::Valid),
         case(AggregateOpSpec::Missing),
         case(AggregateOpSpec::Distinct),
-        case(AggregateOpSpec::Sum),
+        // Re-enable after updating to Vega 5.26.2 when sum([null]) behavior will matche
+        // case(AggregateOpSpec::Sum),
         case(AggregateOpSpec::Mean),
         case(AggregateOpSpec::Average),
         case(AggregateOpSpec::Min),