From 3c3bdbf87a1129dfe4c2d5b4961f0d8345b6fcdd Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:00:18 +0000 Subject: [PATCH 1/2] Fix MAXPOOL and MEDIANPOOL throwing on non-tiling dimensions MAXPOOL and MEDIANPOOL computed the output array size without checking that the pooling window actually tiles the input range. When the range dimensions, reduced by the window size, were not whole multiples of the stride, the kernel read past the last row of the input and an uncaught TypeError escaped the interpreter and the public API. - Validate the window size and the stride against the range dimensions and return #VALUE! (ErrorMessage.PoolDimensions) instead. - Require the window size and the stride to be positive integers, so that a zero, negative or fractional value returns #NUM! instead of throwing. - Reuse the new predicate in maxpoolArraySize to keep the predicted array size and the runtime validation in sync. - Document the constraint in the built-in functions guide and in the method JSDoc. --- CHANGELOG.md | 1 + docs/guide/built-in-functions.md | 8 + src/error-message.ts | 1 + src/interpreter/plugin/MatrixPlugin.ts | 76 +++++++-- .../interpreter/matrix-plugin-pooling.spec.ts | 152 ++++++++++++++++++ 5 files changed, 227 insertions(+), 11 deletions(-) create mode 100644 test/unit/interpreter/matrix-plugin-pooling.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 46de676d5e..1b4dd21e4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Fixed the behavior of `MATCH`, `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions when the search range contained empty cells. [#1697](https://github.com/handsontable/hyperformula/pull/1697) - Fixed the `VLOOKUP`, `HLOOKUP`, and `XLOOKUP` functions to return `0` instead of an empty value when the matched cell in the result range is empty. [#1697](https://github.com/handsontable/hyperformula/pull/1697) - Fixed the page freezing when entering a long string of digits containing a non-digit character near the end (e.g. `012...789a` or `012...789 123`) into a cell. [#1520](https://github.com/handsontable/hyperformula/issues/1520) +- Fixed the MAXPOOL and MEDIANPOOL functions throwing an uncaught `TypeError` instead of returning the `#VALUE!` error when the range dimensions are not a whole multiple of the window size and the stride. ## [3.3.0] - 2026-05-20 diff --git a/docs/guide/built-in-functions.md b/docs/guide/built-in-functions.md index 51846e0978..f902b86d43 100644 --- a/docs/guide/built-in-functions.md +++ b/docs/guide/built-in-functions.md @@ -348,6 +348,14 @@ Total number of functions: **{{ $page.functionsCount }}** | MAXPOOL | Calculates a smaller range which is a maximum of a Window_size, in a given Range, for every Stride element. | MAXPOOL(Range, Window_size, Stride) | | TRANSPOSE | Transposes the rows and columns of an array. | TRANSPOSE(Array) | +::: tip +`MAXPOOL` and `MEDIANPOOL` require the pooling window to tile `Range` exactly: `Window_size` must be a positive +integer that is not greater than either dimension of `Range`, `Stride` must be a positive integer (defaulting to +`Window_size`), and both dimensions of `Range`, reduced by `Window_size`, must be whole multiples of `Stride`. +Otherwise, the functions return the `#VALUE!` error. For example, `MAXPOOL(A1:C3, 2)` returns `#VALUE!`, because a +window of 2 cells cannot tile a range of 3 cells, while `MAXPOOL(A1:C3, 2, 1)` and `MAXPOOL(A1:D4, 2)` are valid. +::: + ### Operator | Function ID | Description | Syntax | diff --git a/src/error-message.ts b/src/error-message.ts index 5e3afdbeab..4b80ee06fa 100644 --- a/src/error-message.ts +++ b/src/error-message.ts @@ -12,6 +12,7 @@ export class ErrorMessage { public static EmptyArg = 'Empty function argument.' public static EmptyArray = 'Empty array not allowed.' public static ArrayDimensions = 'Array dimensions are not compatible.' + public static PoolDimensions = 'Range dimensions are not compatible with the window size and the stride.' public static NoSpaceForArrayResult = 'No space for array result.' public static ValueSmall = 'Value too small.' public static ValueLarge = 'Value too large.' diff --git a/src/interpreter/plugin/MatrixPlugin.ts b/src/interpreter/plugin/MatrixPlugin.ts index b2d329d95e..c2e5535780 100644 --- a/src/interpreter/plugin/MatrixPlugin.ts +++ b/src/interpreter/plugin/MatrixPlugin.ts @@ -9,6 +9,7 @@ import {ErrorMessage} from '../../error-message' import {AstNodeType, ProcedureAst} from '../../parser' import {InterpreterState} from '../InterpreterState' import {InternalScalarValue, InterpreterValue} from '../InterpreterValue' +import {Maybe} from '../../Maybe' import {SimpleRangeValue} from '../../SimpleRangeValue' import {FunctionArgumentType, FunctionPlugin, FunctionPluginTypecheck, ImplementedFunctions} from './FunctionPlugin' @@ -37,6 +38,43 @@ function arraySizeForPoolFunction(inputArray: ArraySize, windowSize: number, str ) } +/** + * Checks whether a square pooling window tiles the input array exactly, so that no window reaches outside of it. + * + * The window has to fit inside the input array and both of its dimensions, reduced by the window size, + * have to be whole multiples of the stride. + * + * @param inputArray - dimensions of the pooled input array + * @param windowSize - side length of the square pooling window + * @param stride - distance between the top-left corners of two consecutive windows + */ +function isPoolWindowFittingInputArray(inputArray: ArraySize, windowSize: number, stride: number): boolean { + return windowSize <= inputArray.width + && windowSize <= inputArray.height + && (inputArray.width - windowSize) % stride === 0 + && (inputArray.height - windowSize) % stride === 0 +} + +/** + * Validates the arguments shared by the pooling functions (MAXPOOL, MEDIANPOOL). + * + * @param matrix - the pooled input range + * @param windowSize - side length of the square pooling window + * @param stride - distance between the top-left corners of two consecutive windows + * @returns a {@link CellError} describing the violated constraint, or `undefined` when the arguments are valid + */ +function poolFunctionArgumentsError(matrix: SimpleRangeValue, windowSize: number, stride: number): Maybe { + if (!matrix.hasOnlyNumbers()) { + return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + } + + if (!isPoolWindowFittingInputArray(matrix.size, windowSize, stride)) { + return new CellError(ErrorType.VALUE, ErrorMessage.PoolDimensions) + } + + return undefined +} + export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypecheck { public static implementedFunctions: ImplementedFunctions = { 'MMULT': { @@ -61,8 +99,8 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech sizeOfResultArrayMethod: 'maxpoolArraySize', parameters: [ {argumentType: FunctionArgumentType.RANGE}, - {argumentType: FunctionArgumentType.NUMBER}, - {argumentType: FunctionArgumentType.NUMBER, optionalArg: true}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1, optionalArg: true}, ], vectorizationForbidden: true, }, @@ -71,8 +109,8 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech sizeOfResultArrayMethod: 'medianpoolArraySize', parameters: [ {argumentType: FunctionArgumentType.RANGE}, - {argumentType: FunctionArgumentType.NUMBER}, - {argumentType: FunctionArgumentType.NUMBER, optionalArg: true}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1}, + {argumentType: FunctionArgumentType.INTEGER, minValue: 1, optionalArg: true}, ], vectorizationForbidden: true, }, @@ -110,10 +148,19 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech return arraySizeForMultiplication(left, right) } + /** + * Corresponds to MAXPOOL(Range, Window_size, Stride). + * + * Reduces the input range to the maximum value of every window of `Window_size` x `Window_size` cells, + * moving the window by `Stride` cells. The window has to fit inside the range and the range dimensions, + * reduced by the window size, have to be whole multiples of the stride. Otherwise, the function + * returns the #VALUE! error. + */ public maxpool(ast: ProcedureAst, state: InterpreterState): InterpreterValue { return this.runFunction(ast.args, state, this.metadata('MAXPOOL'), (matrix: SimpleRangeValue, windowSize: number, stride: number = windowSize) => { - if (!matrix.hasOnlyNumbers()) { - return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + const argumentsError = poolFunctionArgumentsError(matrix, windowSize, stride) + if (argumentsError !== undefined) { + return argumentsError } const outputSize = arraySizeForPoolFunction(matrix.size, windowSize, stride) @@ -133,10 +180,19 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech }) } + /** + * Corresponds to MEDIANPOOL(Range, Window_size, Stride). + * + * Reduces the input range to the median value of every window of `Window_size` x `Window_size` cells, + * moving the window by `Stride` cells. The window has to fit inside the range and the range dimensions, + * reduced by the window size, have to be whole multiples of the stride. Otherwise, the function + * returns the #VALUE! error. + */ public medianpool(ast: ProcedureAst, state: InterpreterState): InterpreterValue { return this.runFunction(ast.args, state, this.metadata('MEDIANPOOL'), (matrix: SimpleRangeValue, windowSize: number, stride: number = windowSize) => { - if (!matrix.hasOnlyNumbers()) { - return new CellError(ErrorType.VALUE, ErrorMessage.NumberRange) + const argumentsError = poolFunctionArgumentsError(matrix, windowSize, stride) + if (argumentsError !== undefined) { + return argumentsError } const outputSize = arraySizeForPoolFunction(matrix.size, windowSize, stride) @@ -227,9 +283,7 @@ export class MatrixPlugin extends FunctionPlugin implements FunctionPluginTypech } } - if (window > array.width || window > array.height - || stride > window - || (array.width - window) % stride !== 0 || (array.height - window) % stride !== 0) { + if (stride > window || !isPoolWindowFittingInputArray(array, window, stride)) { return ArraySize.error() } diff --git a/test/unit/interpreter/matrix-plugin-pooling.spec.ts b/test/unit/interpreter/matrix-plugin-pooling.spec.ts new file mode 100644 index 0000000000..6ba6599eec --- /dev/null +++ b/test/unit/interpreter/matrix-plugin-pooling.spec.ts @@ -0,0 +1,152 @@ +import {CellError, DetailedCellError, ErrorType, HyperFormula} from '../../../src' +import {SimpleCellAddress, simpleCellAddress} from '../../../src/Cell' +import {Config} from '../../../src/Config' +import {ErrorMessage} from '../../../src/error-message' + +const colNumber = (input: string): number => + input.split('').reduce((currentColumn, nextLetter) => currentColumn * 26 + (nextLetter.toUpperCase().charCodeAt(0) - 64), 0) - 1 + +const adr = (stringAddress: string, sheet: number = 0): SimpleCellAddress => { + const result = /^([A-Za-z]+)([0-9]+)$/.exec(stringAddress)! + return simpleCellAddress(sheet, colNumber(result[1]), Number(result[2]) - 1) +} + +const detailedError = (errorType: ErrorType, message?: string): DetailedCellError => + new DetailedCellError(new CellError(errorType, message), new Config().translationPackage.getErrorTranslation(errorType)) + +const poolDimensionsError = () => detailedError(ErrorType.VALUE, ErrorMessage.PoolDimensions) + +const squareRange3x3 = [ + [3, 1, 2], + [9, 7, 8], + [5, 4, 6], +] + +const squareRange4x4 = [ + [1, 2, 10, 20], + [3, 4, 30, 40], + [5, 6, 7, 8], + [7, 8, 9, 10], +] + +const buildEngineWithSheet = (sheetContent: number[][]): { engine: HyperFormula, sheetId: number } => { + const engine = HyperFormula.buildEmpty() + const sheetId = engine.getSheetId(engine.addSheet('Sheet1'))! + engine.setSheetContent(sheetId, sheetContent) + return {engine, sheetId} +} + +describe.each(['MAXPOOL', 'MEDIANPOOL'])('Function %s dimension validation', (functionName: string) => { + it('returns an error when the range dimensions are not a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 2)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error in a cell when the range dimensions are not a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + engine.setCellContents(adr('A5', sheetId), [[`=${functionName}(A1:C3, 2)`]]) + + expect(engine.getCellValue(adr('A5', sheetId))).toEqualError(poolDimensionsError()) + }) + + it('returns an error when only one of the range dimensions is a whole multiple of the window size', () => { + const {engine, sheetId} = buildEngineWithSheet([ + [1, 2, 3, 4], + [5, 6, 7, 8], + [9, 10, 11, 12], + ]) + + expect(engine.calculateFormula(`=${functionName}(A1:D3, 2)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the window is larger than the range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 4)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the stride makes the last window reach outside of the range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula(`=${functionName}(A1:C3, 2, 3)`, sheetId)).toEqualError(poolDimensionsError()) + }) + + it('returns an error when the window size is not a positive integer', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula(`=${functionName}(A1:D4, 0)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, -2)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2.5)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.IntegerExpected)) + }) + + it('returns an error when the stride is not a positive integer', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, 0)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, -1)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) + expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, 1.5)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.IntegerExpected)) + }) +}) + +describe('Function MAXPOOL', () => { + it('pools a range whose dimensions are a whole multiple of the window size into maximums', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + expect(engine.calculateFormula('=MAXPOOL(A1:D4, 2)', sheetId)).toEqual([ + [4, 40], + [8, 10], + ]) + }) + + it('pools a range whose dimensions are a whole multiple of a custom stride', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula('=MAXPOOL(A1:C3, 2, 1)', sheetId)).toEqual([ + [9, 8], + [9, 8], + ]) + }) + + it('pools a range with a window covering the whole range', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) + + expect(engine.calculateFormula('=MAXPOOL(A1:C3, 3)', sheetId)).toEqual(9) + }) + + it('spills the pooled maximums into cells', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + engine.setCellContents(adr('A6', sheetId), [['=MAXPOOL(A1:D4, 2)']]) + + expect(engine.getCellValue(adr('A6', sheetId))).toEqual(4) + expect(engine.getCellValue(adr('B6', sheetId))).toEqual(40) + expect(engine.getCellValue(adr('A7', sheetId))).toEqual(8) + expect(engine.getCellValue(adr('B7', sheetId))).toEqual(10) + }) +}) + +describe('Function MEDIANPOOL', () => { + it('pools a range whose dimensions are a whole multiple of the window size into medians', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + const result = engine.calculateFormula('=MEDIANPOOL(A1:D4, 2)', sheetId) as number[][] + + expect(result[0][0]).toBeCloseTo(2.5) + expect(result[0][1]).toBeCloseTo(25) + expect(result[1][0]).toBeCloseTo(6.5) + expect(result[1][1]).toBeCloseTo(8.5) + }) + + it('spills the pooled medians into cells', () => { + const {engine, sheetId} = buildEngineWithSheet(squareRange4x4) + + engine.setCellContents(adr('A6', sheetId), [['=MEDIANPOOL(A1:D4, 2)']]) + + expect(engine.getCellValue(adr('A6', sheetId))).toBeCloseTo(2.5) + expect(engine.getCellValue(adr('B6', sheetId))).toBeCloseTo(25) + expect(engine.getCellValue(adr('A7', sheetId))).toBeCloseTo(6.5) + expect(engine.getCellValue(adr('B7', sheetId))).toBeCloseTo(8.5) + }) +}) From 725b84de0c98de5e74feee24d943de1116bd198a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 11:09:01 +0000 Subject: [PATCH 2/2] Replace describe.each with a loop in the pooling tests The browser test suite runs under Karma and Jasmine, which has no describe.each, so the Jest-only helper made the whole Karma run fail with "TypeError: describe.each is not a function". A plain forEach over the function names works in both runners. --- test/unit/interpreter/matrix-plugin-pooling.spec.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/unit/interpreter/matrix-plugin-pooling.spec.ts b/test/unit/interpreter/matrix-plugin-pooling.spec.ts index 6ba6599eec..f9c57ec8a0 100644 --- a/test/unit/interpreter/matrix-plugin-pooling.spec.ts +++ b/test/unit/interpreter/matrix-plugin-pooling.spec.ts @@ -16,6 +16,8 @@ const detailedError = (errorType: ErrorType, message?: string): DetailedCellErro const poolDimensionsError = () => detailedError(ErrorType.VALUE, ErrorMessage.PoolDimensions) +const POOL_FUNCTION_NAMES = ['MAXPOOL', 'MEDIANPOOL'] + const squareRange3x3 = [ [3, 1, 2], [9, 7, 8], @@ -36,7 +38,8 @@ const buildEngineWithSheet = (sheetContent: number[][]): { engine: HyperFormula, return {engine, sheetId} } -describe.each(['MAXPOOL', 'MEDIANPOOL'])('Function %s dimension validation', (functionName: string) => { +// A plain loop instead of `describe.each`, which is not available in the Jasmine-based browser test suite. +POOL_FUNCTION_NAMES.forEach((functionName: string) => describe(`Function ${functionName} dimension validation`, () => { it('returns an error when the range dimensions are not a whole multiple of the window size', () => { const {engine, sheetId} = buildEngineWithSheet(squareRange3x3) @@ -88,7 +91,7 @@ describe.each(['MAXPOOL', 'MEDIANPOOL'])('Function %s dimension validation', (fu expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, -1)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.ValueSmall)) expect(engine.calculateFormula(`=${functionName}(A1:D4, 2, 1.5)`, sheetId)).toEqualError(detailedError(ErrorType.NUM, ErrorMessage.IntegerExpected)) }) -}) +})) describe('Function MAXPOOL', () => { it('pools a range whose dimensions are a whole multiple of the window size into maximums', () => {