Make FieldTypes a Map to preserve column order#9279
Conversation
Fixes #9269. Column names that parse as non-negative integers (e.g. `"2000"`, `"2010"`) were hoisted to the front in numeric order when `FieldTypes` was a plain `Record<string, DataType>` — ECMAScript's `OrdinaryOwnPropertyKeys` iterates integer-indexed keys before string keys. `Map` preserves insertion order for all keys and keeps O(1) lookup.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Bundle ReportChanges will increase total bundle size by 225.11kB (0.9%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: marimo-esmAssets Changed:
Files in
Files in
Files in
|
There was a problem hiding this comment.
Pull request overview
Fix column-order instability for digit-only column names (e.g. "2000", "2010") by changing FieldTypes from a plain object to an ordered Map, avoiding ECMAScript integer-key reordering during iteration.
Changes:
- Change
FieldTypestoMap<string, DataType>and updatetoFieldTypesaccordingly. - Update Data Editor and column summary/chart codepaths to use
MapAPIs (.get,.has, iteration). - Update/add Vitest coverage, including a regression test for insertion order with digit-string column names.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/plugins/impl/data-editor/glide-data-editor.tsx | Iterates columnFields as a Map and switches lookups to .get/.has. |
| frontend/src/plugins/impl/data-editor/data-utils.ts | Updates modifyColumnFields to operate on Map (insert/remove/rename). |
| frontend/src/plugins/impl/data-editor/tests/data-utils.test.ts | Adjusts fixtures/assertions for Map-based FieldTypes. |
| frontend/src/plugins/impl/DataEditorPlugin.tsx | Initializes columnFields as a Map and converts to Record only for Vega CSV parsing. |
| frontend/src/components/data-table/types.ts | Redefines FieldTypes as Map and updates toFieldTypes. |
| frontend/src/components/data-table/column-summary/column-summary.tsx | Guards stats rendering when type is missing. |
| frontend/src/components/data-table/column-summary/chart-spec-model.tsx | Uses Map.get for fieldTypes and handles missing type when building specs. |
| frontend/src/components/data-table/tests/types.test.ts | Adds regression test ensuring toFieldTypes preserves insertion order for digit-string keys. |
| frontend/src/components/data-table/tests/chart-spec-model.test.ts | Updates test fixtures to use Map-based FieldTypes. |
| const columns: ModifiedGridColumn[] = useMemo(() => { | ||
| const columns: ModifiedGridColumn[] = []; | ||
| for (const [columnName, fieldType] of Object.entries(columnFields)) { | ||
| for (const [columnName, fieldType] of columnFields) { | ||
| const editable = | ||
| editableColumns === "all" || editableColumns.includes(columnName); |
There was a problem hiding this comment.
Switching columnFields to a Map preserves insertion order for columns, but index-based column operations (e.g. delete) can now diverge from removeColumn, which picks the key by Object.keys(row) ordering. For digit-string column names ("2000"), Object.keys will reorder keys, so deleting column N may remove the wrong property from each row. Prefer removing columns by name (derive const columnName = columns[columnIdx].title) and delete that property, or change removeColumn to accept a column name / explicit key order rather than relying on Object.keys.
| const { error } = useAsyncData(async () => { | ||
| const withoutExternalTypes = toFieldTypes(props.fieldTypes ?? []); | ||
|
|
||
| // If we already have the data, return it |
There was a problem hiding this comment.
This comment says "If we already have the data, return it", but the async loader always loads/sets localData and doesn't have an early-return path based on existing state. Either remove the misleading line or implement the intended guard (e.g. skip loading when data is already populated and props.data hasn’t changed).
| // If we already have the data, return it | |
| // Use the provided rows directly when `props.data` is already an array. |
| const newEntries: Array<[string, DataType]> = [ | ||
| ...entries.slice(0, columnIdx), | ||
| [newColumnName, dataType || "string"], |
There was a problem hiding this comment.
In the rename case, if dataType is omitted this code sets the renamed column’s type to "string". However BulkEdit.Rename edits (applied in glide-data-editor.tsx) don’t include a dataType, so rehydrating/applying initial rename edits will silently change the column’s type (e.g. number -> string). Consider preserving the existing type at columnIdx when dataType is undefined, only overriding when an explicit dataType is provided.
| const newEntries: Array<[string, DataType]> = [ | |
| ...entries.slice(0, columnIdx), | |
| [newColumnName, dataType || "string"], | |
| const existingDataType = entries[columnIdx]?.[1]; | |
| const newEntries: Array<[string, DataType]> = [ | |
| ...entries.slice(0, columnIdx), | |
| [newColumnName, dataType ?? existingDataType ?? "string"], |
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.23.2-dev63 |
Fixes #9269.
Column names that parse as non-negative integers (e.g.
"2000","2010") were hoisted to the front in numeric order whenFieldTypeswas a plainRecord<string, DataType>since ECMAScript'sOrdinaryOwnPropertyKeysiterates integer-indexed keys before string keys.