-
Notifications
You must be signed in to change notification settings - Fork 168
Feature/choosecols function #1734
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.
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
base: develop
Are you sure you want to change the base?
Changes from 6 commits
86892f1
8e0ff87
9b2901b
f915ac4
8a03fb4
0df4d1f
77a01d9
f9d182e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,17 +3,58 @@ | |
| * Copyright (c) 2025 Handsoncode. All rights reserved. | ||
| */ | ||
|
|
||
| import {AbsoluteCellRange} from '../../AbsoluteCellRange' | ||
| import {ArraySize} from '../../ArraySize' | ||
| import {CellError, ErrorType} from '../../Cell' | ||
| import {ErrorMessage} from '../../error-message' | ||
| import {AstNodeType, ProcedureAst} from '../../parser' | ||
| import {Ast, AstNodeType, ProcedureAst} from '../../parser' | ||
| import {coerceScalarToBoolean} from '../ArithmeticHelper' | ||
| import {InterpreterState} from '../InterpreterState' | ||
| import {InternalScalarValue, InterpreterValue} from '../InterpreterValue' | ||
| import {getRawValue, InternalScalarValue, InterpreterValue} from '../InterpreterValue' | ||
| import {SimpleRangeValue} from '../../SimpleRangeValue' | ||
| import {FunctionArgumentType, FunctionPlugin, FunctionPluginTypecheck, ImplementedFunctions} from './FunctionPlugin' | ||
|
|
||
| /** A CHOOSECOLS index classified without evaluating a formula expression. */ | ||
| type ChooseColsLiteralIndex = | ||
| | {kind: 'value', value: number} | ||
| | {kind: 'invalid'} | ||
| | {kind: 'unresolved'} | ||
|
|
||
| export class ArrayPlugin extends FunctionPlugin implements FunctionPluginTypecheck<ArrayPlugin> { | ||
| /** | ||
| * Classifies an index literal for static CHOOSECOLS result-size prediction. | ||
| * | ||
| * @param {Ast} argument - The column-index argument to inspect without evaluating formulas. | ||
| * @returns {ChooseColsLiteralIndex} A coerced literal value, an invalid marker, or an unresolved marker. | ||
| */ | ||
| private parseChooseColsLiteralIndex(argument: Ast): ChooseColsLiteralIndex { | ||
| if (argument.type === AstNodeType.NUMBER) { | ||
| return {kind: 'value', value: Math.trunc(argument.value)} | ||
| } | ||
|
|
||
| if (argument.type === AstNodeType.STRING) { | ||
| const coercedValue = this.arithmeticHelper.coerceToMaybeNumber(argument.value) | ||
| if (coercedValue === undefined) { | ||
| return {kind: 'invalid'} | ||
| } | ||
| return {kind: 'value', value: Math.trunc(getRawValue(coercedValue))} | ||
| } | ||
|
|
||
| if (argument.type === AstNodeType.PLUS_UNARY_OP && argument.value.type === AstNodeType.NUMBER) { | ||
| return {kind: 'value', value: Math.trunc(argument.value.value)} | ||
| } | ||
|
|
||
| if (argument.type === AstNodeType.MINUS_UNARY_OP && argument.value.type === AstNodeType.NUMBER) { | ||
| return {kind: 'value', value: Math.trunc(-argument.value.value)} | ||
| } | ||
|
|
||
| if (argument.type === AstNodeType.PARENTHESIS) { | ||
| return this.parseChooseColsLiteralIndex(argument.expression) | ||
| } | ||
|
|
||
| return {kind: 'unresolved'} | ||
| } | ||
|
cursor[bot] marked this conversation as resolved.
|
||
|
|
||
| public static implementedFunctions: ImplementedFunctions = { | ||
| 'ARRAYFORMULA': { | ||
| method: 'arrayformula', | ||
|
|
@@ -43,6 +84,17 @@ export class ArrayPlugin extends FunctionPlugin implements FunctionPluginTypeche | |
| ], | ||
| repeatLastArgs: 1, | ||
| }, | ||
| 'CHOOSECOLS': { | ||
| method: 'choosecols', | ||
| sizeOfResultArrayMethod: 'choosecolsArraySize', | ||
| enableArrayArithmeticForArguments: true, | ||
| parameters: [ | ||
| {argumentType: FunctionArgumentType.RANGE}, | ||
| {argumentType: FunctionArgumentType.NUMBER}, | ||
| ], | ||
| repeatLastArgs: 1, | ||
| vectorizationForbidden: true, | ||
| }, | ||
| 'VSTACK': { | ||
| method: 'vstack', | ||
| sizeOfResultArrayMethod: 'vstackArraySize', | ||
|
|
@@ -166,6 +218,122 @@ export class ArrayPlugin extends FunctionPlugin implements FunctionPluginTypeche | |
| return new ArraySize(width, height) | ||
| } | ||
|
|
||
| /** | ||
| * Corresponds to CHOOSECOLS(array, col_num1, [col_num2], ...). | ||
| * | ||
| * Returns the requested source columns in argument order. Positive indexes | ||
| * count from the left, negative indexes count from the right, and duplicate | ||
| * indexes duplicate their columns in the result. | ||
| * | ||
| * @param {ProcedureAst} ast - The parsed function-call AST node. | ||
| * @param {InterpreterState} state - The current interpreter evaluation state. | ||
| * @returns {InterpreterValue} The selected source columns or a spreadsheet error. | ||
| */ | ||
| public choosecols(ast: ProcedureAst, state: InterpreterState): InterpreterValue { | ||
| return this.runFunction(ast.args, state, this.metadata('CHOOSECOLS'), | ||
| (range: SimpleRangeValue, ...columnNumbers: number[]) => { | ||
| const sourceWidth = range.width() | ||
| const sourceHeight = range.height() | ||
|
|
||
| if (sourceHeight === 0 || sourceWidth === 0) { | ||
| return new CellError(ErrorType.NA, ErrorMessage.EmptyRange) | ||
| } | ||
|
|
||
| const columnIndexes = columnNumbers.map(columnNumber => Math.trunc(columnNumber)) | ||
|
|
||
| if (columnIndexes.some(columnIndex => | ||
| !Number.isFinite(columnIndex) || columnIndex === 0 || Math.abs(columnIndex) > sourceWidth | ||
| )) { | ||
| return new CellError(ErrorType.VALUE, ErrorMessage.IndexBounds) | ||
| } | ||
|
|
||
| const zeroBasedColumnIndexes = columnIndexes.map(columnIndex => | ||
| columnIndex > 0 ? columnIndex - 1 : sourceWidth + columnIndex | ||
| ) | ||
|
|
||
| const sourceRange = range.range | ||
| const startsBelowFirstRow = sourceRange !== undefined | ||
| && !Number.isFinite(sourceRange.height()) | ||
| && state.formulaAddress.row !== 0 | ||
|
|
||
| if (startsBelowFirstRow) { | ||
| return new CellError(ErrorType.SPILL, ErrorMessage.NoSpaceForArrayResult) | ||
| } | ||
|
|
||
| if (sourceRange !== undefined) { | ||
| const selectedColumns = zeroBasedColumnIndexes.map(columnIndex => { | ||
| const columnRange = AbsoluteCellRange.spanFrom( | ||
| sourceRange.getAddress(columnIndex, 0), | ||
| 1, | ||
| sourceHeight, | ||
| ) | ||
| return SimpleRangeValue.onlyRange(columnRange, this.dependencyGraph).data | ||
| }) | ||
| const result = Array.from({length: sourceHeight}, (_, row) => | ||
| selectedColumns.map(column => column[row][0]) | ||
| ) | ||
| return SimpleRangeValue.onlyValues(result) | ||
| } | ||
|
|
||
| const result = range.data.map(row => | ||
| zeroBasedColumnIndexes.map(columnIndex => row[columnIndex]) | ||
| ) | ||
| return SimpleRangeValue.onlyValues(result) | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } | ||
| ) | ||
| } | ||
|
|
||
| /** | ||
| * Predicts the CHOOSECOLS spill size from the source height and index count. | ||
| * | ||
| * Invalid literals are rejected before spill allocation. A whole-column | ||
| * result is valid only in the first output row, then its source range | ||
| * supplies the materialized spill height. | ||
| * | ||
| * @param {ProcedureAst} ast - The parsed function-call AST node. | ||
| * @param {InterpreterState} state - The current interpreter evaluation state. | ||
| * @returns {ArraySize} The predicted result dimensions or an invalid size. | ||
| */ | ||
| public choosecolsArraySize(ast: ProcedureAst, state: InterpreterState): ArraySize { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same finding as on #1722's TAKE:
|
||
| if (ast.args.length < 2) { | ||
| return ArraySize.error() | ||
| } | ||
|
|
||
| const metadata = this.metadata('CHOOSECOLS') | ||
| const sourceSize = this.arraySizeForAst( | ||
| ast.args[0], | ||
| new InterpreterState(state.formulaAddress, state.arraysFlag || (metadata?.enableArrayArithmeticForArguments ?? false)), | ||
| ) | ||
|
|
||
| const startsBelowFirstRow = !Number.isFinite(sourceSize.height) && state.formulaAddress.row !== 0 | ||
| const sourceRange = ast.args[0].type === AstNodeType.COLUMN_RANGE | ||
| ? AbsoluteCellRange.fromAstOrUndef(ast.args[0], state.formulaAddress) | ||
| : undefined | ||
| const effectiveHeight = !Number.isFinite(sourceSize.height) && sourceRange !== undefined | ||
| ? sourceRange.effectiveHeight(this.dependencyGraph) | ||
| : sourceSize.height | ||
|
|
||
| if (startsBelowFirstRow || effectiveHeight < 1) { | ||
| return ArraySize.error() | ||
|
cursor[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| for (const argument of ast.args.slice(1)) { | ||
| const index = this.parseChooseColsLiteralIndex(argument) | ||
| if ( | ||
| index.kind === 'invalid' | ||
| || (index.kind === 'value' && ( | ||
| !Number.isFinite(index.value) | ||
| || index.value === 0 | ||
| || Math.abs(index.value) > sourceSize.width | ||
| )) | ||
| ) { | ||
| return ArraySize.error() | ||
| } | ||
| } | ||
|
|
||
| return new ArraySize(ast.args.length - 1, effectiveHeight) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Finite height breaks column spillsHigh Severity
Reviewed by Cursor Bugbot for commit 0df4d1f. Configure here.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I reran this against the current PR commit ( One clarification: updating a source cell does reevaluate the formula, but it does not rerun the array-size predictor or rebuild the formula vertex. Test setupI started with three values in column A and then extended the sheet’s used height by adding a fourth value: hf.setCellContents(
{ sheet: dataSheet, col: 0, row: 3 },
40,
)I tested the following cross-sheet formulas: I repeated the test using same-sheet references: ResultAll three functions throw the same error when the new value extends the used height of the source range: For the same-sheet tests, The following control cases succeed:
Could you rerun the source-growth check against Based on these results, they do not currently expand successfully after the used height of a whole-column source increases. It also appears that preserving This may therefore require an engine-level change to spill allocation or predicted-size retention -- or another mechanism that allows an existing array formula vertex to grow during recalculation. |
||
| } | ||
|
|
||
| /** | ||
| * Corresponds to VSTACK(array1, [array2], ...) | ||
| * | ||
|
|
||


Uh oh!
There was an error while loading. Please reload this page.