From f389c31ac95c1b4c32c028e06de5a13f29a1569e Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Fri, 25 Jul 2025 17:55:53 +0530 Subject: [PATCH 1/2] remove bool from sum variant in AggContext --- core/types.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/types.rs b/core/types.rs index 56822c34b..5cbe08152 100644 --- a/core/types.rs +++ b/core/types.rs @@ -498,7 +498,7 @@ impl Value { #[derive(Debug, Clone, PartialEq)] pub enum AggContext { Avg(Value, Value), // acc and count - Sum(Value, bool), // acc and has_non_numeric + Sum(Value), Count(Value), Max(Option), Min(Option), @@ -522,7 +522,7 @@ impl AggContext { pub fn final_value(&self) -> &Value { match self { Self::Avg(acc, _count) => acc, - Self::Sum(acc, _) => acc, + Self::Sum(acc) => acc, Self::Count(count) => count, Self::Max(max) => max.as_ref().unwrap_or(&NULL), Self::Min(min) => min.as_ref().unwrap_or(&NULL), @@ -596,7 +596,7 @@ impl PartialOrd for AggContext { fn partial_cmp(&self, other: &AggContext) -> Option { match (self, other) { (Self::Avg(a, _), Self::Avg(b, _)) => a.partial_cmp(b), - (Self::Sum(a, _), Self::Sum(b, _)) => a.partial_cmp(b), + (Self::Sum(a), Self::Sum(b)) => a.partial_cmp(b), (Self::Count(a), Self::Count(b)) => a.partial_cmp(b), (Self::Max(a), Self::Max(b)) => a.partial_cmp(b), (Self::Min(a), Self::Min(b)) => a.partial_cmp(b), From 4f8027990d3ff4d07222ba7eafa45a8955c138a3 Mon Sep 17 00:00:00 2001 From: bit-aloo Date: Fri, 25 Jul 2025 17:59:19 +0530 Subject: [PATCH 2/2] detach the sum and total logic from using has_non_numeric flag --- core/vdbe/execute.rs | 68 ++++++++++++++------------------------------ 1 file changed, 21 insertions(+), 47 deletions(-) diff --git a/core/vdbe/execute.rs b/core/vdbe/execute.rs index cdd210c3c..f629fae04 100644 --- a/core/vdbe/execute.rs +++ b/core/vdbe/execute.rs @@ -3065,12 +3065,12 @@ pub fn op_agg_step( AggFunc::Avg => { Register::Aggregate(AggContext::Avg(Value::Float(0.0), Value::Integer(0))) } - AggFunc::Sum => Register::Aggregate(AggContext::Sum(Value::Null, false)), + AggFunc::Sum => Register::Aggregate(AggContext::Sum(Value::Null)), AggFunc::Total => { // The result of total() is always a floating point value. // No overflow error is ever raised if any prior input was a floating point value. // Total() never throws an integer overflow. - Register::Aggregate(AggContext::Sum(Value::Float(0.0), false)) + Register::Aggregate(AggContext::Sum(Value::Float(0.0))) } AggFunc::Count | AggFunc::Count0 => { Register::Aggregate(AggContext::Count(Value::Integer(0))) @@ -3128,32 +3128,25 @@ pub fn op_agg_step( state.registers[*acc_reg], *acc_reg ); }; - let AggContext::Sum(acc, has_non_numeric) = agg.borrow_mut() else { + let AggContext::Sum(acc) = agg.borrow_mut() else { unreachable!(); }; match col { Register::Value(owned_value) => { match owned_value { - Value::Integer(_) | Value::Float(_) => { - // Promote accumulator to float if mixing integer and float - if matches!((&*acc, &owned_value), (Value::Integer(_), Value::Float(_))) - || matches!( - (&*acc, &owned_value), - (Value::Float(_), Value::Integer(_)) - ) - { - if let Value::Integer(i) = acc { - *acc = Value::Float(*i as f64); - } - } - *acc += owned_value; - } + Value::Integer(_) | Value::Float(_) => match acc { + Value::Null => *acc = owned_value.clone(), + _ => *acc += owned_value, + }, Value::Null => { // Null values are ignored in sum } - _ => { - *has_non_numeric = true; - } + _ => match acc { + Value::Null => *acc = Value::Float(0.0), + Value::Integer(i) => *acc = Value::Float(*i as f64 + 0.0), + Value::Float(_) => {} + _ => unreachable!(), + }, } } _ => unreachable!(), @@ -3337,44 +3330,25 @@ pub fn op_agg_final( state.registers[*register] = Register::Value(acc.clone()); } AggFunc::Sum => { - let AggContext::Sum(acc, has_non_numeric) = agg.borrow_mut() else { + let AggContext::Sum(acc) = agg.borrow_mut() else { unreachable!(); }; let value = match acc { - Value::Integer(i) => { - if *has_non_numeric { - Value::Float(*i as f64) - } else { - Value::Integer(*i) - } - } - Value::Float(f) => Value::Float(*f), - _ => { - if *has_non_numeric { - // Non-numeric values encountered - Value::Float(0.0) - } else { - // Only NULL values encountered - Value::Null - } - } + Value::Null => Value::Null, + v @ Value::Integer(_) | v @ Value::Float(_) => v.clone(), + _ => unreachable!(), }; state.registers[*register] = Register::Value(value); } AggFunc::Total => { - let AggContext::Sum(acc, has_non_numeric) = agg.borrow_mut() else { + let AggContext::Sum(acc) = agg.borrow_mut() else { unreachable!(); }; let value = match acc { - Value::Integer(i) => { - if *has_non_numeric { - Value::Float(*i as f64) - } else { - Value::Integer(*i) - } - } + Value::Null => Value::Float(0.0), + Value::Integer(i) => Value::Float(*i as f64), Value::Float(f) => Value::Float(*f), - _ => Value::Float(0.0), + _ => unreachable!(), }; state.registers[*register] = Register::Value(value); }