diff --git a/CLAUDE.md b/CLAUDE.md index 607e990..c27ce20 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -556,6 +556,43 @@ Turning a region back into slices re-derives, per cell, the same filters Perspec `pf_iter` is not a col_meta column, so it is stripped when a slice is built: two cells differing only by iter band produce the same effective slice. Duplicates are collapsed before the request — without that, `apply_mode: each` would apply the same change twice. +**A blank dimension value is a value.** Both blanks occur in this data and they +are different rows — on `fc_osm_skinny_34`, `partgroup` is null on 51,964 rows +and `''` on 111,696 — so `null` and `''` stay distinct from the clicked cell +through to the WHERE clause, where one becomes `IS NULL` and the other `= ''`. +Only `undefined` means absent, which is how a subtotal row (a shorter row path) +and a collapsed column level still read as "not part of the slice". + +They used to be dropped, and a dropped key does not narrow the write — it widens +it. Clicking the blank `partgroup` node under `ALTMAN PLANTS` selected the +customer's whole $27.6M rather than the $6.2M under the node clicked, and the +ledger agreed with it, because the panel totals through the same filter list. +The source of the drop is the **engine**: `getCellConfig` builds the click's +filter list with `pivot_value ? [pivot, '==', pivot_value] : undefined`, so the +blank is gone before `perspective-click` is dispatched — and being a falsy test +it takes a dimension whose value is `0` with it. `cellFilters()` in +`Forecast.jsx` therefore re-derives both axes from `detail.row.__ROW_PATH__` and +the clicked column name rather than trusting that list; the click and a dragged +region now go through the same derivation. The ledger filters a null with +`is null` rather than `== null`, since the engine matches on type and a null +threshold against a string column is the same dropped predicate again. + +`buildWhere()` renders a null as `IS NULL`; it used to fall through `esc()` to +`= ''`, which selected *the other blank* — the slice named one node and the +operation wrote the other. In a list, the null arm is ORed on rather than folded +in, because `IN (NULL)` is never true. + +The panel prints `(null)` and `(blank)` rather than nothing, and `—` only when +the column is genuinely absent from the slice: a slice isolated to the blank node +and one the column plays no part in are opposite statements and looked identical. + +**A blank on the column axis cannot be isolated.** A column name is its values +joined with `|`, and a null joins as the empty string, so by the time the name +reaches the client the two blanks are the same text. Those cells are refused — +with a message, and counted once for a drag — rather than widened, since a slice +missing a split_by level means every value of it. Filtering on the column +instead (`scope`, which carries `is null`) is the way to reach them. + **The pivot's own filter travels alongside, as `scope`.** A slice is `{col: value}` and can only mean equality, so a view narrowed to `sseas_e <= 2027` has no slice form at all — the filter is read off diff --git a/lib/sql_generator.js b/lib/sql_generator.js index 1b6de06..e227759 100644 --- a/lib/sql_generator.js +++ b/lib/sql_generator.js @@ -604,9 +604,25 @@ function buildWhere(slice, dimCols, versionId) { throw err; } if (!allowed.has(col)) continue; - if (Array.isArray(val)) { - const escaped = val.map(v => esc(v)); - parts.push(`"${col}" IN ('${escaped.join("', '")}')`); + // A blank dimension value is a value. Both blanks exist in this data and + // they are different rows -- on fc_osm_skinny_34, partgroup is null on + // 51,964 and '' on 111,696 -- so a null has to become IS NULL. It used + // to fall through esc() to `= ''`, which selected the *other* blank: + // the slice named one node and the operation wrote the other. + if (val === null) { + parts.push(`"${col}" IS NULL`); + } else if (Array.isArray(val)) { + const nulls = val.some(v => v === null); + const escaped = val.filter(v => v !== null).map(v => esc(v)); + const inList = escaped.length ? `"${col}" IN ('${escaped.join("', '")}')` : null; + // an IN list cannot carry a null -- `IN (NULL)` is never true -- so + // the null arm is ORed on rather than folded in + parts.push( + nulls && inList ? `("${col}" IS NULL OR ${inList})` + : nulls ? `"${col}" IS NULL` + : inList ? inList + : 'FALSE' + ); } else { parts.push(`"${col}" = '${esc(val)}'`); } diff --git a/ui/src/components/OperationPanel.jsx b/ui/src/components/OperationPanel.jsx index b66fa07..a033f50 100644 --- a/ui/src/components/OperationPanel.jsx +++ b/ui/src/components/OperationPanel.jsx @@ -27,9 +27,19 @@ function fmtDelta(n, decimals = 2) { return `${sign}${fmtNum(Math.abs(n), decimals)}` } +// A blank dimension value has to read as a value, or a slice isolated to the +// blank node is indistinguishable from one the column is absent from -- and the +// two mean opposite things. Both blanks occur in the same column here, so they +// are named apart: null is a missing value, '' is a present empty one. +function dispVal(v) { + if (v === null || v === undefined) return '(null)' + if (v === '') return '(blank)' + return String(v) +} + function sliceLabel(s) { const entries = Object.entries(s) - return entries.length ? entries.map(([k, v]) => `${k}=${v}`).join(' · ') : '—' + return entries.length ? entries.map(([k, v]) => `${k}=${dispVal(v)}`).join(' · ') : '—' } // A light grouping with an optional caption — a rule between blocks in the @@ -161,7 +171,9 @@ function SelectionList({ slices, viewScope = [], currentTotals, onRemove, onClea
{k} = - {v} + + {dispVal(v)} +
))} @@ -1019,9 +1031,10 @@ function DimForm({ dimCols, setObj, setSet, slices, lookupDerivedCols, dimMember {dimCols.map(c => { + const inSlice = c.cname in first const cur = multi - ? (new Set(slices.map(s => s[c.cname])).size > 1 ? '(varies)' : (first[c.cname] ?? '—')) - : (first[c.cname] ?? '—') + ? (new Set(slices.map(s => s[c.cname])).size > 1 ? '(varies)' : (inSlice ? dispVal(first[c.cname]) : '—')) + : (inSlice ? dispVal(first[c.cname]) : '—') return ( {c.label || c.cname} diff --git a/ui/src/views/Forecast.jsx b/ui/src/views/Forecast.jsx index 54634fc..cee9086 100644 --- a/ui/src/views/Forecast.jsx +++ b/ui/src/views/Forecast.jsx @@ -349,7 +349,6 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio const cfg = await viewer.save() const view = await viewer.getView() const groupBy = cfg.group_by || [] - const splitBy = cfg.split_by || [] const base = cfg.filter || [] const rows = await view.to_json({ start_row: win.start_row, end_row: win.end_row }) if (!rows.length) return [] @@ -357,32 +356,21 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio const c0 = win.start_col ?? 0 const c1 = win.end_col ?? userKeys.length const out = [] + // cells whose column-axis value is blank, which cannot be isolated (see + // cellFilters) -- counted so the drag can say why it skipped them once, + // rather than per cell + let blankCols = 0 for (let i = 0; i < rows.length; i++) { const path = rows[i].__ROW_PATH__ || [] - const rowFilters = groupBy - .map((col, ix) => (path[ix] ? [col, '==', path[ix]] : null)) - .filter(Boolean) // the grand-total row resolves to no dimension at all, which would mean // "the whole version" — never what dragging over it is asking for - if (groupBy.length && !rowFilters.length) continue + if (groupBy.length && !path.length) continue for (let c = c0; c < c1; c++) { const key = userKeys[c] if (!key) continue - // A column name is dimension values joined by | with the measure last, and - // only as many values as the axis currently shows -- collapse the column - // hierarchy and the deeper levels are simply absent. Mapping split_by - // positionally over every segment therefore read the measure as a value - // for the first collapsed dimension: a bucket subtotal came back as - // smon_e = 'sales_usd', which matches no row, so the operation silently - // had nothing to act on. Drop the measure, then map over what is left. - const segs = key.split('|').slice(0, -1) - const colFilters = splitBy - .slice(0, segs.length) - .map((col, ix) => { - const v = segs[ix] - return (v && !META_COL_RE.test(v)) ? [col, '==', v] : null - }) - .filter(Boolean) + const { rowFilters, colFilters, colBlank } = cellFilters(cfg, rows[i], key) + if (colBlank) { blankCols++; continue } + if (groupBy.length && !rowFilters.length) continue const slice = sliceFromFilters([...base, ...rowFilters, ...colFilters], (cfg.columns || []).filter(Boolean)) if (!Object.keys(slice).length) continue @@ -390,6 +378,9 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio out.push({ slice, area: { x0: c, x1: c, y0: y, y1: y } }) } } + if (blankCols) { + flash(`${blankCols} cell${blankCols > 1 ? 's' : ''} skipped: a blank column-axis value cannot be isolated`, 'error') + } return out } @@ -515,7 +506,12 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio // a cell inside the filtered view cannot contradict it, so a repeated // column is the same predicate twice and harmless .filter(([col]) => !viewFilter.some(f => f[0] === col)) - .map(([col, val]) => [col, '==', typed(col, val)]), + // A null is filtered with `is null`, not `== null`: the engine matches + // on type, and a null threshold against a string column is the same + // dropped predicate that widened the slice in the first place. The + // ledger has to total the rows the operation will write, and the + // operation writes IS NULL. + .map(([col, val]) => (val === null ? [col, 'is null'] : [col, '==', typed(col, val)])), ] const view = await tableRef.current.view({ filter: filters }) const rows = await view.to_json() @@ -999,15 +995,33 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (!detail.row) return const config = await viewer.save() if (!(config.group_by || []).length) return - const s = sliceFromFilters((detail.config || {}).filter || [], - (config.columns || []).filter(Boolean)) + // the datagrid wipes its highlight on mousedown, so keep this cell's + // rectangle alongside the slice and re-apply the lot (see the effect above) + const cell = downCellRef.current + // detail.config.filter has already lost any blank group key -- the engine + // drops a falsy pivot value when it builds that list -- so the slice is + // re-derived from detail.row's own row path, exactly as a dragged region + // is. The clicked column comes from the mousedown's cell, since the event + // names only the measure. Without a cell there is nothing to index the + // column axis with, so fall back to the event's own list. + const colKey = cell ? userKeysOf(detail.row)[cell.x] : undefined + const measures = (config.columns || []).filter(Boolean) + let s + if (cell) { + const { rowFilters, colFilters, colBlank } = cellFilters(config, detail.row, colKey) + if (colBlank) { + flash('A blank column-axis value cannot be isolated — filter on it instead', 'error') + return + } + if (!rowFilters.length) return + s = sliceFromFilters([...(config.filter || []), ...rowFilters, ...colFilters], measures) + } else { + s = sliceFromFilters((detail.config || {}).filter || [], measures) + } if (!Object.keys(s).length) return // the CustomEvent carries no modifier flags, so read them off the // mousedown that produced it (captured on window below) const additive = modifierRef.current - // the datagrid wipes its highlight on mousedown, so keep this cell's - // rectangle alongside the slice and re-apply the lot (see the effect above) - const cell = downCellRef.current const k = sliceKey(s) const next = additive ? toggleSlice(slicesRef.current, s) : [s] if (!additive) areasRef.current.clear() @@ -1699,6 +1713,8 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio if (COMPUTED_SLICE_COLS.has(k)) { out[k] = v; continue } if (dimCols.has(k)) { out[k] = v; continue } if (dateCols.has(k)) { + // a blank date node is a null date, not epoch zero + if (v === null || v === '') { out[k] = null; continue } const ms = Number(v) out[k] = isFinite(ms) ? new Date(ms).toISOString().slice(0, 10) : v } @@ -2361,19 +2377,83 @@ const META_COL_RE = /^__(?:ROW_PATH(?:_\d+)?|ID|GROUPING_ID)__$/ // over a column name that no longer has that many segments. Left in, the slice // asks for a month equal to a measure, matches nothing, and the operation // silently has no rows to act on. +// A blank dimension value is a value, not an absence. `null` and `''` are both +// real and both present in this data -- partgroup has 51,964 of one and 111,696 +// of the other -- so they stay distinct all the way to the WHERE clause, where +// null becomes IS NULL and '' becomes = ''. Dropping either is what widened a +// selection: with partgroup gone from the slice, clicking the blank node under +// ALTMAN PLANTS selected all $27.6M of the customer rather than the $6.2M under +// the node clicked. +// +// Only `undefined` means absent, which is how a subtotal row (a shorter row +// path) and a collapsed column level still read as "this level is not part of +// the slice". function sliceFromFilters(filters, measures = []) { const measureSet = new Set(measures) const s = {} for (const f of filters) { if (!Array.isArray(f)) continue const [col, op, val] = f - if (op !== '==' || val == null) continue - if (measureSet.has(String(val))) continue - s[col] = String(val) + if (op !== '==' || val === undefined) continue + if (val !== null && measureSet.has(String(val))) continue + s[col] = val === null ? null : String(val) } return s } +// The dimension filters for one cell, re-derived from the row path and the column +// name rather than taken from the click event. +// +// `getCellConfig` in the datagrid builds its filter list with +// `pivot_value ? [pivot, '==', pivot_value] : undefined`, so **the engine itself +// drops a blank group key** before the event is dispatched -- and a falsy one, so +// a dimension whose value is 0 goes the same way. The row path is the raw truth +// and carries the blank, so the slice is built from that instead. +// +// A blank on the *column* axis cannot be recovered at all: a column name is its +// values joined with '|', and a null joins as the empty string, so the two are +// the same text by the time it reaches us. Those cells are refused rather than +// widened -- `colBlank` says so -- since a slice missing a split_by level means +// every value of it. +function cellFilters(cfg, row, colKey) { + const groupBy = cfg.group_by || [] + const splitBy = cfg.split_by || [] + const path = row?.__ROW_PATH__ || [] + const rowFilters = groupBy + .map((col, ix) => (ix < path.length && path[ix] !== undefined ? [col, '==', path[ix]] : null)) + .filter(Boolean) + let colFilters = [] + let colBlank = false + if (colKey != null) { + // A column name is dimension values joined by | with the measure last, and + // only as many values as the axis currently shows -- collapse the column + // hierarchy and the deeper levels are simply absent. Mapping split_by + // positionally over every segment read the measure as a value for the first + // collapsed dimension: a bucket subtotal came back as smon_e = 'sales_usd', + // which matches no row, so the operation silently had nothing to act on. + // Drop the measure, then map over what is left. + const segs = String(colKey).split('|') + segs.pop() + colFilters = splitBy + .map((col, ix) => { + const v = segs[ix] + // absent: the column hierarchy is collapsed above this level + if (v === undefined) return null + // the measure name read positionally as a dimension value + if (META_COL_RE.test(v)) return null + if (v === '') { colBlank = true; return null } + return [col, '==', v] + }) + .filter(Boolean) + } + return { rowFilters, colFilters, colBlank } +} + +// the user keys of a to_json row, in the order the grid lays its columns out +function userKeysOf(row) { + return Object.keys(row || {}).filter(k => !META_COL_RE.test(k)) +} + // slices are compared by value — the same row clicked twice is the same slice function sliceKey(s) { return JSON.stringify(Object.keys(s).sort().map(k => [k, s[k]]))