Skip to content

Commit a3c4644

Browse files
giswqsgiswqs
authored
Preserve text field types in attribute summaries (#1937)
* fix: preserve text types in attribute summaries * test: narrow nullable field statistics * fix: apply source-aware field inference * fix: keep mixed field values type-safe * Address Claude review feedback - Gate numeric-string coercion per column instead of per value in `coerceNumericStringRows`: a column is converted only when its numeric-looking values clear the same `>= 2 && >= populated / 2` threshold the summaries apply. A mostly-free-text column with a stray "3" / "3.0" now keeps those cells verbatim, so distinct-value counts and top-value labels are no longer collapsed or reformatted for a field that still classifies as text. - Memoize the geojson-backed `attributeRows` in `AttributeTable` so the analysis adapters stop rebuilding every row's properties on unrelated re-renders (scroll virtualization, filter typing, selection). The DuckDB branch stays unmemoized because `getDuckDBLayerRows` reads a mutable store; delimited-text layers, the ones that pay for the coercion pass, take the geojson path. - Cover both the new column gating and the two-row coercion case in `tests/attribute-charts.test.ts`.
1 parent 5ad5123 commit a3c4644

9 files changed

Lines changed: 309 additions & 81 deletions

File tree

‎apps/geolibre-desktop/src/components/layout/PrintLayoutDialog.tsx‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ import {
6767
} from "../../lib/print-data-blocks";
6868
import {
6969
categoricalColumns,
70+
coerceNumericStringRows,
7071
numericColumns,
7172
type BarAggregation,
7273
type ChartRow,
@@ -977,10 +978,13 @@ export function PrintLayoutDialog({
977978
() => (tableLayer?.geojson ? layerRows(tableLayer.geojson) : []),
978979
[tableLayer],
979980
);
980-
const chartAllRows = useMemo(
981-
() => (chartLayer?.geojson ? layerRows(chartLayer.geojson) : []),
982-
[chartLayer],
983-
);
981+
const chartAllRows = useMemo(() => {
982+
if (!chartLayer?.geojson) return [];
983+
const rows = layerRows(chartLayer.geojson);
984+
return chartLayer.metadata.sourceKind === "delimited-text"
985+
? coerceNumericStringRows(rows)
986+
: rows;
987+
}, [chartLayer]);
984988
// Per-feature bounds for the page-extent filter, walked once per layer so
985989
// stepping/exporting an N-page atlas does not redo the vertex walk N times
986990
// (the same precompute pattern the atlas page builder uses).

‎apps/geolibre-desktop/src/components/panels/AttributeTable.tsx‎

Lines changed: 49 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,7 @@ import {
106106
type CalcOutputType,
107107
} from "../../lib/attribute-expression";
108108
import { attributeFormErrorMessage } from "../../lib/attribute-form-messages";
109+
import { coerceNumericStringRows } from "../../lib/attribute-charts";
109110
import { computeRowSelection } from "../../lib/attribute-selection";
110111
import { RESERVED_PROPERTY_KEYS } from "../../lib/field-collection";
111112
import {
@@ -436,6 +437,7 @@ export function AttributeTable({ mapControllerRef }: AttributeTableProps) {
436437
const calcExpressionRef = useRef<HTMLTextAreaElement>(null);
437438

438439
const layer = layers.find((l) => l.id === selectedLayerId);
440+
const coerceNumericStrings = layer?.metadata.sourceKind === "delimited-text";
439441
const hasLayer = Boolean(layer);
440442
// Columns materialized by persistent joins are derived data: every save
441443
// re-derives them from the join table, so an edit, rename, or delete here
@@ -459,15 +461,26 @@ export function AttributeTable({ mapControllerRef }: AttributeTableProps) {
459461
() => new Set([...joinDerivedColumns, ...virtualFieldColumns]),
460462
[joinDerivedColumns, virtualFieldColumns],
461463
);
462-
const features = layer?.geojson?.features ?? [];
464+
const features = useMemo(() => layer?.geojson?.features ?? [], [layer?.geojson]);
463465
const isDuckDBLayer = isDuckDBQueryLayer(layer);
464466
const duckdbRows = layer && isDuckDBLayer ? getDuckDBLayerRows(layer.id) : [];
465-
const attributeRows: AttributeTableRow[] = isDuckDBLayer
466-
? duckDBRowsToAttributeRows(duckdbRows)
467-
: features.map((feature, index) => ({
467+
// Memoized so the analysis adapters below (and everything else keyed on these
468+
// rows) only rebuild when the layer's features actually change, not on every
469+
// render caused by scrolling, filtering or selection. The DuckDB branch stays
470+
// unmemoized because `getDuckDBLayerRows` reads a mutable store and returns a
471+
// fresh array each call; delimited-text layers — the ones that pay for the
472+
// numeric-string adapter below — are geojson-backed and take this path.
473+
const geojsonRows: AttributeTableRow[] = useMemo(
474+
() =>
475+
features.map((feature, index) => ({
468476
featureId: String(feature.id ?? index),
469477
properties: (feature.properties ?? {}) as Record<string, unknown>,
470-
}));
478+
})),
479+
[features],
480+
);
481+
const attributeRows: AttributeTableRow[] = isDuckDBLayer
482+
? duckDBRowsToAttributeRows(duckdbRows)
483+
: geojsonRows;
471484
const hasAttributeSource = Boolean(layer?.geojson || isDuckDBLayer);
472485
// Add Vector Layer (geojson-mode) layers render from a MapLibre source the
473486
// control owns, and their `layer.geojson` is dropped when a project is saved.
@@ -612,14 +625,32 @@ export function AttributeTable({ mapControllerRef }: AttributeTableProps) {
612625
// O(1) lookups for the multi-selection while rendering thousands of rows.
613626
const selectedIdSet = useMemo(() => new Set(selectedFeatureIds), [selectedFeatureIds]);
614627

615-
const filterLower = attributeFilter.toLowerCase();
616-
const filtered = attributeRows.filter(({ properties, featureId }) => {
617-
// "Show Selected Features" restricts the table to the current selection.
618-
if (featureView === "selected" && !selectedIdSet.has(featureId)) return false;
619-
if (!filterLower) return true;
620-
const props = JSON.stringify(properties).toLowerCase();
621-
return featureId.includes(filterLower) || props.includes(filterLower);
622-
});
628+
const filtered = useMemo(() => {
629+
const filterLower = attributeFilter.toLowerCase();
630+
return attributeRows.filter(({ properties, featureId }) => {
631+
// "Show Selected Features" restricts the table to the current selection.
632+
if (featureView === "selected" && !selectedIdSet.has(featureId)) return false;
633+
if (!filterLower) return true;
634+
const props = JSON.stringify(properties).toLowerCase();
635+
return featureId.includes(filterLower) || props.includes(filterLower);
636+
});
637+
}, [attributeFilter, attributeRows, featureView, selectedIdSet]);
638+
// Delimited-text imports preserve every cell as a string. Adapt only the rows
639+
// sent to analysis dialogs, leaving the table and exported source data intact.
640+
const adaptAnalysisRows = coerceNumericStrings && (chartOpen || statsOpen || explorerOpen);
641+
const analysisRows = useMemo(
642+
() => (adaptAnalysisRows ? coerceNumericStringRows(attributeRows) : attributeRows),
643+
[adaptAnalysisRows, attributeRows],
644+
);
645+
const analysisFilteredRows = useMemo(
646+
() =>
647+
adaptAnalysisRows
648+
? filtered.length === attributeRows.length
649+
? analysisRows
650+
: coerceNumericStringRows(filtered)
651+
: filtered,
652+
[adaptAnalysisRows, analysisRows, attributeRows.length, filtered],
653+
);
623654
const sorted = [...filtered].sort((a, b) => {
624655
const aValue = sort.key === "__featureId" ? a.featureId : a.properties[sort.key];
625656
const bValue = sort.key === "__featureId" ? b.featureId : b.properties[sort.key];
@@ -2320,23 +2351,23 @@ export function AttributeTable({ mapControllerRef }: AttributeTableProps) {
23202351
<AttributeChartDialog
23212352
open={chartOpen}
23222353
onOpenChange={setChartOpen}
2323-
rows={attributeRows}
2354+
rows={analysisRows}
23242355
columns={discoveredColumns}
23252356
layerName={layer?.name ?? ""}
23262357
/>
23272358
<AttributeStatsDialog
23282359
open={statsOpen}
23292360
onOpenChange={setStatsOpen}
2330-
rows={attributeRows}
2331-
filteredRows={filtered}
2361+
rows={analysisRows}
2362+
filteredRows={analysisFilteredRows}
23322363
columns={discoveredColumns}
23332364
layerName={layer?.name ?? ""}
23342365
/>
23352366
<ColumnExplorerDialog
23362367
open={explorerOpen}
23372368
onOpenChange={setExplorerOpen}
2338-
rows={attributeRows}
2339-
filteredRows={filtered}
2369+
rows={analysisRows}
2370+
filteredRows={analysisFilteredRows}
23402371
columns={discoveredColumns}
23412372
layerName={layer?.name ?? ""}
23422373
/>

‎apps/geolibre-desktop/src/hooks/useLayerChartData.ts‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { isDuckDBQueryLayer, useAppStore, type GeoLibreLayer } from "@geolibre/core";
22
import { getDuckDBLayerRows } from "@geolibre/plugins";
33
import { useMemo } from "react";
4-
import type { ChartRow } from "../lib/attribute-charts";
4+
import { coerceNumericStringRows, type ChartRow } from "../lib/attribute-charts";
55

66
export interface LayerChartData {
77
/** Attribute rows for charting (just the property bag each chart needs). */
@@ -35,11 +35,15 @@ function buildLayerChartData(layer: GeoLibreLayer | null): LayerChartData {
3535
// Mirror the attribute table's two row sources: DuckDB query layers fetch
3636
// their rows from the plugin's cache, every other vector layer reads its
3737
// features straight off `layer.geojson`.
38-
const rows: ChartRow[] = isDuckDBQueryLayer(layer)
38+
const sourceRows: ChartRow[] = isDuckDBQueryLayer(layer)
3939
? getDuckDBLayerRows(layer.id).map((row) => ({ properties: row.properties }))
4040
: (layer.geojson?.features ?? []).map((feature) => ({
4141
properties: (feature.properties ?? {}) as Record<string, unknown>,
4242
}));
43+
const rows =
44+
layer.metadata.sourceKind === "delimited-text"
45+
? coerceNumericStringRows(sourceRows)
46+
: sourceRows;
4347

4448
const keys = new Set<string>();
4549
for (const row of rows) {
@@ -58,8 +62,8 @@ function buildLayerChartData(layer: GeoLibreLayer | null): LayerChartData {
5862
}
5963

6064
/**
61-
* Resolve a layer id to the rows, columns, and name a chart widget renders
62-
* from. Recomputes when the layer record changes (e.g. attribute edits replace
65+
* Resolve a layer id to the rows, columns, and name a chart widget uses.
66+
* Recomputes when the layer record changes (e.g. attribute edits replace
6367
* `layer.geojson`). DuckDB query rows come from the plugin cache and are read
6468
* once per layer-identity change, matching the attribute table's behavior.
6569
*

‎apps/geolibre-desktop/src/lib/attribute-charts.ts‎

Lines changed: 86 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,80 @@ export function toFiniteNumber(value: unknown): number | null {
4040
return null;
4141
}
4242

43+
/**
44+
* Test whether a stored field value contributes to numeric type inference.
45+
* Only explicit numeric values qualify. Callers handling string-only data
46+
* sources adapt their analysis rows with {@link coerceNumericStringRows} first.
47+
*/
48+
export function isNumericFieldValue(value: unknown): value is number {
49+
return typeof value === "number" && Number.isFinite(value);
50+
}
51+
52+
/** True when a string encodes an integer with meaningful leading zeroes. */
53+
function hasLeadingZeroes(value: string): boolean {
54+
return /^[+-]?0\d+$/.test(value.trim());
55+
}
56+
57+
/** True for common delimited-text headers that conventionally hold identifiers. */
58+
function isIdentifierFieldName(key: string): boolean {
59+
const normalized = key.replace(/([a-z\d])([A-Z])/g, "$1_$2").toLowerCase();
60+
return /(^|[\s_-])(id|code|fips|zip|zipcode|postal)([\s_-]|$)/.test(normalized);
61+
}
62+
63+
/**
64+
* Convert numeric-looking strings in analysis rows without changing the source
65+
* data. Delimited-text layers use this adapter because their parser preserves
66+
* every cell as a string, including measurements that summaries should treat as
67+
* numeric. Because delimited text carries no declared field types, common
68+
* identifier headers and integer strings with leading zeroes remain text.
69+
*
70+
* The decision is per column, not per value: a column is converted only when its
71+
* numeric-looking values would carry it past the same threshold the summaries
72+
* apply (at least two of them, and at least half of the populated rows). A
73+
* mostly-free-text column holding a stray `"3.0"` therefore keeps that cell
74+
* verbatim, so its distinct-value counts and top-value labels stay faithful to
75+
* what the file says.
76+
*/
77+
export function coerceNumericStringRows(rows: ChartRow[]): ChartRow[] {
78+
const textKeys = new Set<string>();
79+
const populatedCounts = new Map<string, number>();
80+
const numericCounts = new Map<string, number>();
81+
for (const row of rows) {
82+
for (const [key, value] of Object.entries(row.properties)) {
83+
if (value == null || value === "") continue;
84+
populatedCounts.set(key, (populatedCounts.get(key) ?? 0) + 1);
85+
if (typeof value === "string" && (isIdentifierFieldName(key) || hasLeadingZeroes(value))) {
86+
textKeys.add(key);
87+
continue;
88+
}
89+
// Values already stored as numbers count toward the threshold too, since
90+
// they are what the summaries will see after this pass.
91+
const countsAsNumeric =
92+
isNumericFieldValue(value) || (typeof value === "string" && toFiniteNumber(value) !== null);
93+
if (countsAsNumeric) numericCounts.set(key, (numericCounts.get(key) ?? 0) + 1);
94+
}
95+
}
96+
97+
const numericKeys = new Set<string>();
98+
for (const [key, numeric] of numericCounts) {
99+
if (textKeys.has(key)) continue;
100+
if (numeric >= 2 && numeric >= (populatedCounts.get(key) ?? 0) / 2) numericKeys.add(key);
101+
}
102+
if (numericKeys.size === 0) return rows;
103+
104+
return rows.map((row) => {
105+
let properties: Record<string, unknown> | null = null;
106+
for (const [key, value] of Object.entries(row.properties)) {
107+
if (!numericKeys.has(key) || typeof value !== "string") continue;
108+
const numeric = toFiniteNumber(value);
109+
if (numeric === null) continue;
110+
properties ??= { ...row.properties };
111+
properties[key] = numeric;
112+
}
113+
return properties ? { ...row, properties } : row;
114+
});
115+
}
116+
43117
/**
44118
* The distinct non-empty values of a category field, sorted for display. Used
45119
* by the selector widget to build its list of value chips.
@@ -109,7 +183,7 @@ export function numericColumns(rows: ChartRow[], columns: string[]): string[] {
109183
const raw = row.properties[key];
110184
if (raw == null || raw === "") continue;
111185
nonNull += 1;
112-
if (toFiniteNumber(raw) !== null) numeric += 1;
186+
if (isNumericFieldValue(raw)) numeric += 1;
113187
}
114188
return numeric >= 2 && numeric >= nonNull / 2;
115189
});
@@ -119,8 +193,8 @@ export function numericColumns(rows: ChartRow[], columns: string[]): string[] {
119193
export function numericValues(rows: ChartRow[], key: string): number[] {
120194
const values: number[] = [];
121195
for (const row of rows) {
122-
const next = toFiniteNumber(row.properties[key]);
123-
if (next !== null) values.push(next);
196+
const value = row.properties[key];
197+
if (isNumericFieldValue(value)) values.push(value);
124198
}
125199
return values;
126200
}
@@ -233,9 +307,9 @@ export function computeScatter(
233307
let yMin = 0;
234308
let yMax = 0;
235309
for (const row of rows) {
236-
const x = toFiniteNumber(row.properties[xKey]);
237-
const y = toFiniteNumber(row.properties[yKey]);
238-
if (x === null || y === null) continue;
310+
const x = row.properties[xKey];
311+
const y = row.properties[yKey];
312+
if (!isNumericFieldValue(x) || !isNumericFieldValue(y)) continue;
239313
if (all.length === 0) {
240314
xMin = xMax = x;
241315
yMin = yMax = y;
@@ -356,8 +430,8 @@ export function computeBar(
356430
const group = groups.get(label) ?? { count: 0, sum: 0, numericCount: 0 };
357431
group.count += 1;
358432
if (aggregation !== "count" && valueKey) {
359-
const value = toFiniteNumber(row.properties[valueKey]);
360-
if (value !== null) {
433+
const value = row.properties[valueKey];
434+
if (isNumericFieldValue(value)) {
361435
group.sum += value;
362436
group.numericCount += 1;
363437
}
@@ -428,8 +502,8 @@ export function computePie(
428502
const group = groups.get(label) ?? { count: 0, sum: 0 };
429503
group.count += 1;
430504
if (aggregation !== "count" && valueKey) {
431-
const value = toFiniteNumber(row.properties[valueKey]);
432-
if (value !== null) group.sum += value;
505+
const value = row.properties[valueKey];
506+
if (isNumericFieldValue(value)) group.sum += value;
433507
}
434508
groups.set(label, group);
435509
}
@@ -497,8 +571,8 @@ export function computeLine(rows: ChartRow[], key: string): LineResult | null {
497571
let max = -Infinity;
498572
let index = 0;
499573
for (const row of rows) {
500-
const value = toFiniteNumber(row.properties[key]);
501-
if (value !== null) {
574+
const value = row.properties[key];
575+
if (isNumericFieldValue(value)) {
502576
points.push({ index, value });
503577
if (value < min) min = value;
504578
if (value > max) max = value;

‎apps/geolibre-desktop/src/lib/attribute-stats.ts‎

Lines changed: 22 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@
33
* detect whether a field reads as numeric or text and compute a compact summary
44
* for it (count / nulls / min / max / mean / median / std / sum / unique for
55
* numbers; count / nulls / unique / most-frequent values for text). Kept free of
6-
* any rendering or React so they can be unit-tested in isolation, and built on
7-
* the same `{ properties }` rows and numeric coercion the Charts panel uses so
8-
* the two panels agree on what counts as a number.
6+
* any rendering or React so they can be unit-tested in isolation. Field type
7+
* detection respects the primitive types stored in `{ properties }`, so a text
8+
* field is not reclassified just because every string parses as a number.
9+
* String-only sources can explicitly retain their numeric inference behavior.
910
*/
1011

11-
import { numericColumns, toFiniteNumber, type ChartRow } from "./attribute-charts";
12+
import { isNumericFieldValue, type ChartRow } from "./attribute-charts";
1213

1314
export interface NumericFieldStats {
1415
kind: "numeric";
@@ -148,18 +149,27 @@ export function computeTextStats(
148149
}
149150

150151
/**
151-
* Summary statistics for one field, choosing the numeric or text shape from the
152-
* same heuristic the Charts panel uses (`numericColumns`): a field counts as
153-
* numeric when enough of its populated rows parse as finite numbers. Numeric
154-
* fields fold their blank and non-numeric row counts into the result. Returns
155-
* null when `key` is not present, so callers can show an empty state.
152+
* Summary statistics for one field, choosing the numeric or text shape from its
153+
* stored primitive values. A field counts as numeric when at least two populated
154+
* rows contain finite JavaScript numbers and those values make up at least half
155+
* of the populated rows. Numeric-looking strings alone therefore remain text
156+
* unless the caller adapts a string-only source before summarizing it. Numeric
157+
* fields fold their blank and non-numeric row counts into the result.
156158
*/
157159
export function computeFieldStats(
158160
rows: ChartRow[],
159161
key: string,
160162
topCount: number = DEFAULT_TOP_VALUES,
161163
): FieldStats | null {
162-
const isNumeric = numericColumns(rows, [key]).length > 0;
164+
let numeric = 0;
165+
let populated = 0;
166+
for (const row of rows) {
167+
const raw = row.properties[key];
168+
if (raw == null || raw === "") continue;
169+
populated += 1;
170+
if (isNumericFieldValue(raw)) numeric += 1;
171+
}
172+
const isNumeric = numeric >= 2 && numeric >= populated / 2;
163173
if (!isNumeric) return computeTextStats(rows, key, topCount);
164174

165175
const values: number[] = [];
@@ -171,9 +181,8 @@ export function computeFieldStats(
171181
nulls += 1;
172182
continue;
173183
}
174-
const next = toFiniteNumber(raw);
175-
if (next === null) nonNumeric += 1;
176-
else values.push(next);
184+
if (isNumericFieldValue(raw)) values.push(raw);
185+
else nonNumeric += 1;
177186
}
178187
return computeNumericStats(values, nulls, nonNumeric);
179188
}

0 commit comments

Comments
 (0)