Skip to content

Make SUM and PRODUCT functions more consistent with Excel #236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Make SUM and PRODUCT functions more consistent with Excel #236

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1133793
Add additional PRODUCT function tests
gian-hancock Jan 9, 2025
0ebac54
Add additional SUM function tests
gian-hancock Jan 9, 2025
fc13532
Add tests xlsx compatibility tests for SUM and PRODUCT function
gian-hancock Jan 9, 2025
f144754
Fix SUM and PRODUCT function excel compatibility
gian-hancock Jan 9, 2025
8174d06
Update comments in PRODUCT_SUM xlsx test
gian-hancock Jan 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion base/src/cast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ impl Model {
self.cast_to_number(result, cell)
}

fn cast_to_number(
pub(crate) fn cast_to_number(
&mut self,
result: CalcResult,
cell: CellReferenceIndex,
Expand Down
107 changes: 95 additions & 12 deletions base/src/functions/mathematical.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,50 @@ impl Model {
CalcResult::Number(result)
}

/// SUM(number1, [number2], ...)
///
/// The SUM function adds values. You can add individual values, cell references or ranges or a mix of all three.
///
/// # Details
/// Generally each argument is evaluated and the result is converted to a number. However there are some special
/// cases:
/// - **Args evaluating to CalcResult::Range**: The sum of all the numbers in the range is calculated. No
/// conversions are performed, non-numeric values are ignored and errors are propagated.
/// - **Node::ReferenceKind args**: Similar to ranges, conversions are not performed on the evaulated value of the
/// referenced cell and errors are propagated.
/// - **Args evaluating to CalcResult::Error**: Errors are propagated.
pub(crate) fn fn_sum(&mut self, args: &[Node], cell: CellReferenceIndex) -> CalcResult {
if args.is_empty() {
return CalcResult::new_args_number_error(cell);
}

let mut result = 0.0;
for arg in args {
// Handle reference special case. Behaviour of a reference is equivalent to a range of one cell.
let arg = match arg {
Node::ReferenceKind {
column,
absolute_column,
row,
absolute_row,
sheet_index,
sheet_name,
} => &Node::RangeKind {
sheet_name: sheet_name.clone(),
sheet_index: *sheet_index,
absolute_row1: *absolute_row,
absolute_column1: *absolute_column,
row1: *row,
column1: *column,
absolute_row2: *absolute_row,
absolute_column2: *absolute_column,
row2: *row,
column2: *column,
},
_ => arg,
};

match self.evaluate_node_in_context(arg, cell) {
CalcResult::Number(value) => result += value,
CalcResult::Range { left, right } => {
if left.sheet != right.sheet {
return CalcResult::new_error(
Expand Down Expand Up @@ -170,26 +205,66 @@ impl Model {
}
}
error @ CalcResult::Error { .. } => return error,
_ => {
// We ignore booleans and strings
calc_result => {
let cast_result = self.cast_to_number(calc_result, cell);
match cast_result {
Ok(f) => result += f,
Err(s) => return s,
}
}
};
}
CalcResult::Number(result)
}

/// PRODUCT(number1, [number2], ...)
///
/// The PRODUCT function multiplies values. You can add individual values, cell references or ranges or a mix of all
/// three.
///
/// # Details
/// Generally each argument is evaluated and the result is converted to a number. However there are some special
/// cases:
/// - **Args evaluating to CalcResult::Range**: The product of all the numbers in the range is calculated. No
/// conversions are performed, non-numeric values are ignored and errors are propagated.
/// - **Node::ReferenceKind args**: Similar to ranges, conversions are not performed on the evaulated value of the
/// referenced cell and errors are propagated.
/// - **Args evaluating to CalcResult::Error**: Errors are propagated.
pub(crate) fn fn_product(&mut self, args: &[Node], cell: CellReferenceIndex) -> CalcResult {
if args.is_empty() {
return CalcResult::new_args_number_error(cell);
}
let mut result = 1.0;
let mut seen_value = false;

for arg in args {
match self.evaluate_node_in_context(arg, cell) {
CalcResult::Number(value) => {
seen_value = true;
result *= value;
}
// Handle reference special case. Behaviour of a reference is equivalent to a range of one cell.
let arg = match arg {
Node::ReferenceKind {
column,
absolute_column,
row,
absolute_row,
sheet_index,
sheet_name,
} => &Node::RangeKind {
sheet_name: sheet_name.clone(),
sheet_index: *sheet_index,
absolute_row1: *absolute_row,
absolute_column1: *absolute_column,
row1: *row,
column1: *column,
absolute_row2: *absolute_row,
absolute_column2: *absolute_column,
row2: *row,
column2: *column,
},
_ => arg,
};

let cell_value = self.evaluate_node_in_context(arg, cell);

match cell_value {
CalcResult::Range { left, right } => {
if left.sheet != right.sheet {
return CalcResult::new_error(
Expand All @@ -198,6 +273,8 @@ impl Model {
"Ranges are in different sheets".to_string(),
);
}

// TODO: We should do this for all functions that run through ranges. See fn_sum for more details
let row1 = left.row;
let mut row2 = right.row;
let column1 = left.column;
Expand Down Expand Up @@ -228,11 +305,12 @@ impl Model {
}
for row in row1..row2 + 1 {
for column in column1..(column2 + 1) {
match self.evaluate_cell(CellReferenceIndex {
let cell_value = self.evaluate_cell(CellReferenceIndex {
sheet: left.sheet,
row,
column,
}) {
});
match cell_value {
CalcResult::Number(value) => {
seen_value = true;
result *= value;
Expand All @@ -246,8 +324,13 @@ impl Model {
}
}
error @ CalcResult::Error { .. } => return error,
_ => {
// We ignore booleans and strings
calc_result => {
seen_value = true;
let cast_result = self.cast_to_number(calc_result, cell);
match cast_result {
Ok(f) => result *= f,
Err(s) => return s,
}
}
};
}
Expand Down
109 changes: 109 additions & 0 deletions base/src/test/test_fn_product.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,112 @@ fn test_fn_product_arguments() {
// Error (Incorrect number of arguments)
assert_eq!(model._get_text("A1"), *"#ERROR!");
}

#[test]
fn test_fn_product_text_converted_to_number() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT("1")"#);
model._set("A2", r#"=PRODUCT("1e2")"#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"1");
assert_eq!(model._get_text("A2"), *"100");
}

#[test]
fn test_fn_product_invalid_text() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT("a")"#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"#VALUE!");
}

#[test]
fn test_fn_product_text_in_range_not_converted() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(B1:D1)"#);
model._set("B1", r#"="100""#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"0");
}

#[test]
fn test_fn_product_text_in_reference_not_converted() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(B1)"#);
model._set("B1", r#"="100""#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"0");
}

#[test]
fn test_fn_product_text_in_indirect_reference_not_converted() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(INDIRECT("B1"))"#);
model._set("B1", r#"="100""#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"0");
}

#[test]
fn test_fn_product_text_in_indirect_reference() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(INDIRECT("B1"))"#);
model._set("B1", r#"100"#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"100");
}

#[test]
fn test_fn_product_invalid_text_in_range() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(B1:D1)"#);
model._set("B1", "a");

model.evaluate();

assert_eq!(model._get_text("A1"), *"0");
}

#[test]
fn test_fn_product_invalid_text_in_reference() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(B1)"#);
model._set("B1", r#"a"#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"0");
}

#[test]
fn test_fn_product_boolean_values_converted() {
let mut model = new_empty_model();

model._set("A1", r#"=PRODUCT(TRUE)"#);
model._set("A2", r#"=PRODUCT(FALSE)"#);

model.evaluate();

assert_eq!(model._get_text("A1"), *"1");
assert_eq!(model._get_text("A2"), *"0");
}
Loading
Loading