Make a slice mean what it says, and say what cannot move
The phantom: pf_segment and pf_bucket are computed from pf.log when the rows are served, so buildWhere had no column to compare and dropped them. Clicking one bucket's cell and scaling therefore wrote every bucket at that dimension intersection, while the panel showed only the bucket clicked. On the example slice that is 350,524.74 displayed against 503,446.08 written. They resolve exactly, without a new column: the name lives on the log row and every forecast row carries the pf_logid that points at it, so the predicate is pf_logid IN (SELECT id FROM pf.log WHERE <the same expression> = ...). Verified against version 29 -- the clause returns 350,524.74 over 12 rows. Any other pf_ key is now refused rather than skipped, since skipping is the mechanism by which a selection silently widens. pf_iter stays exempt: the client drops it deliberately, two cells differing only by iter band being the same slice. Client side they are ordinary columns in the loaded table, so both the dispatch path and the panel's own totals filter on them directly -- the latter matters as much, or the ledger reconciles against a wider selection than the operation writes. The ledger: excluded rows read "02 - Prior Year · FINAL" in amber rather than "reference · fixed" -- named by the segment a forecaster recognises instead of the iter band that happens to exclude it, and coloured because immovable is a property worth seeing before reading a number. When the whole selection is immovable it now says so in a sentence, where before it printed a row of zeros and left the reason to be inferred from the edit rows failing below. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0cabe9bcd2
commit
50a0bb42aa
@ -512,13 +512,53 @@ function applyTokens(sql, tokens) {
|
|||||||
|
|
||||||
// build a SQL WHERE clause string from a slice object
|
// build a SQL WHERE clause string from a slice object
|
||||||
// only dimension columns are included; unrecognised keys are silently skipped
|
// only dimension columns are included; unrecognised keys are silently skipped
|
||||||
function buildWhere(slice, dimCols) {
|
// pf_segment and pf_bucket are not columns on the forecast table -- they are
|
||||||
|
// computed at read time from the row's pf.log entry -- so a slice naming one
|
||||||
|
// cannot be compared directly. It resolves to a set of log ids instead, which is
|
||||||
|
// exact: the name lives on the log row, and every forecast row carries the
|
||||||
|
// pf_logid that points at it.
|
||||||
|
//
|
||||||
|
// Without this they were dropped from the slice, and clicking a single bucket's
|
||||||
|
// cell scaled every bucket at that dimension intersection while the panel showed
|
||||||
|
// only the one clicked.
|
||||||
|
const COMPUTED_SLICE_COLS = { pf_segment: SEGMENT_EXPR, pf_bucket: BUCKET_EXPR };
|
||||||
|
|
||||||
|
function computedSlicePredicate(col, val, versionId) {
|
||||||
|
if (versionId == null) {
|
||||||
|
const err = new Error(`Cannot filter on ${col} without a version`);
|
||||||
|
err.status = 500;
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
|
const vals = (Array.isArray(val) ? val : [val]).map(v => `'${esc(v)}'`).join(', ');
|
||||||
|
return `pf_logid IN (
|
||||||
|
SELECT l.id
|
||||||
|
FROM pf.log l${VERSION_JOIN}
|
||||||
|
WHERE l.version_id = ${parseInt(versionId)}
|
||||||
|
AND ${COMPUTED_SLICE_COLS[col]} IN (${vals})
|
||||||
|
)`;
|
||||||
|
}
|
||||||
|
|
||||||
|
function buildWhere(slice, dimCols, versionId) {
|
||||||
if (!slice || Object.keys(slice).length === 0) return 'TRUE';
|
if (!slice || Object.keys(slice).length === 0) return 'TRUE';
|
||||||
|
|
||||||
const allowed = new Set(dimCols);
|
const allowed = new Set(dimCols);
|
||||||
const parts = [];
|
const parts = [];
|
||||||
|
|
||||||
for (const [col, val] of Object.entries(slice)) {
|
for (const [col, val] of Object.entries(slice)) {
|
||||||
|
if (COMPUTED_SLICE_COLS[col]) {
|
||||||
|
parts.push(computedSlicePredicate(col, val, versionId));
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
// A pf_ key this does not understand is refused rather than skipped.
|
||||||
|
// Skipping is how a selection silently widened: the operation ran against
|
||||||
|
// everything the dropped key would have excluded. pf_iter is the one
|
||||||
|
// exception -- the client strips it deliberately, since two cells that
|
||||||
|
// differ only by iter band are the same slice.
|
||||||
|
if (col.startsWith('pf_') && col !== 'pf_iter') {
|
||||||
|
const err = new Error(`Slice names ${col}, which cannot be filtered on`);
|
||||||
|
err.status = 400;
|
||||||
|
throw err;
|
||||||
|
}
|
||||||
if (!allowed.has(col)) continue;
|
if (!allowed.has(col)) continue;
|
||||||
if (Array.isArray(val)) {
|
if (Array.isArray(val)) {
|
||||||
const escaped = val.map(v => esc(v));
|
const escaped = val.map(v => esc(v));
|
||||||
@ -535,13 +575,13 @@ function buildWhere(slice, dimCols) {
|
|||||||
// A union of slices cannot be flattened into one IN list per column: slices
|
// A union of slices cannot be flattened into one IN list per column: slices
|
||||||
// {Region:East, State:NY} and {Region:West, State:CA} would become
|
// {Region:East, State:NY} and {Region:West, State:CA} would become
|
||||||
// Region IN (East,West) AND State IN (NY,CA), which also matches East/CA.
|
// Region IN (East,West) AND State IN (NY,CA), which also matches East/CA.
|
||||||
function buildWhereAny(slices, dimCols) {
|
function buildWhereAny(slices, dimCols, versionId) {
|
||||||
const list = (slices || []).filter(s => s && Object.keys(s).length > 0);
|
const list = (slices || []).filter(s => s && Object.keys(s).length > 0);
|
||||||
if (list.length === 0) return 'TRUE';
|
if (list.length === 0) return 'TRUE';
|
||||||
if (list.length === 1) return buildWhere(list[0], dimCols);
|
if (list.length === 1) return buildWhere(list[0], dimCols, versionId);
|
||||||
|
|
||||||
const groups = list
|
const groups = list
|
||||||
.map(s => buildWhere(s, dimCols))
|
.map(s => buildWhere(s, dimCols, versionId))
|
||||||
.filter(w => w !== 'TRUE');
|
.filter(w => w !== 'TRUE');
|
||||||
|
|
||||||
// any slice that reduced to TRUE selects everything, so the union does too
|
// any slice that reduced to TRUE selects everything, so the union does too
|
||||||
@ -627,6 +667,6 @@ function esc(val) {
|
|||||||
return String(val).replace(/'/g, "''");
|
return String(val).replace(/'/g, "''");
|
||||||
}
|
}
|
||||||
|
|
||||||
module.exports = { generateSQL, grainOf,
|
module.exports = { generateSQL, grainOf, COMPUTED_SLICE_COLS,
|
||||||
SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, LABEL_GROUP_COLS, VERSION_JOIN,
|
SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, LABEL_GROUP_COLS, VERSION_JOIN,
|
||||||
ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET, UNLABELED_LOAD, dateGroupsOf, dimPeriodMapOf, dimPeriodJoins, applyTokens, buildWhere, buildWhereAny, buildExcludeClause, buildExcludePredicate, buildSetClause, buildFilterClause, esc };
|
ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET, UNLABELED_LOAD, dateGroupsOf, dimPeriodMapOf, dimPeriodJoins, applyTokens, buildWhere, buildWhereAny, buildExcludeClause, buildExcludePredicate, buildSetClause, buildFilterClause, esc };
|
||||||
|
|||||||
@ -1,6 +1,6 @@
|
|||||||
const express = require('express');
|
const express = require('express');
|
||||||
const { tableFromArrays, tableToIPC } = require('apache-arrow');
|
const { tableFromArrays, tableToIPC } = require('apache-arrow');
|
||||||
const { applyTokens, buildWhere, buildWhereAny, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc,
|
const { applyTokens, buildWhere, buildWhereAny, COMPUTED_SLICE_COLS, buildExcludeClause, buildExcludePredicate, buildSetClause, dateGroupsOf, dimPeriodMapOf, esc,
|
||||||
SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, VERSION_JOIN,
|
SEGMENT_EXPR, BUCKET_EXPR, NOTE_EXPR, VERSION_JOIN,
|
||||||
ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET } = require('../lib/sql_generator');
|
ADJUSTMENT_SEGMENT, ADJUSTMENT_BUCKET } = require('../lib/sql_generator');
|
||||||
const { sessionUser } = require('../lib/auth');
|
const { sessionUser } = require('../lib/auth');
|
||||||
@ -45,9 +45,10 @@ module.exports = function(pool) {
|
|||||||
// rather than distribute an amount, so for them this only decides whether the
|
// rather than distribute an amount, so for them this only decides whether the
|
||||||
// work lands as one log entry or several.
|
// work lands as one log entry or several.
|
||||||
function sliceUnits(slices, ctx, applyMode) {
|
function sliceUnits(slices, ctx, applyMode) {
|
||||||
|
const vid = ctx.version.id;
|
||||||
return applyMode === 'each'
|
return applyMode === 'each'
|
||||||
? slices.map(sl => ({ slices: [sl], where: buildWhere(sl, ctx.filterCols) }))
|
? slices.map(sl => ({ slices: [sl], where: buildWhere(sl, ctx.filterCols, vid) }))
|
||||||
: [{ slices, where: buildWhereAny(slices, ctx.filterCols) }];
|
: [{ slices, where: buildWhereAny(slices, ctx.filterCols, vid) }];
|
||||||
}
|
}
|
||||||
|
|
||||||
// The offset is interpolated into the statement as an interval literal, so a
|
// The offset is interpolated into the statement as an interval literal, so a
|
||||||
@ -73,7 +74,7 @@ module.exports = function(pool) {
|
|||||||
// otherwise reduce to TRUE and apply the operation to the whole version.
|
// otherwise reduce to TRUE and apply the operation to the whole version.
|
||||||
// Refuse rather than let a malformed selection rewrite every row.
|
// Refuse rather than let a malformed selection rewrite every row.
|
||||||
function assertSelective(slices, ctx) {
|
function assertSelective(slices, ctx) {
|
||||||
const allowed = new Set(ctx.filterCols);
|
const allowed = new Set([...ctx.filterCols, ...Object.keys(COMPUTED_SLICE_COLS)]);
|
||||||
slices.forEach((sl, i) => {
|
slices.forEach((sl, i) => {
|
||||||
const hits = Object.keys(sl).filter(k => allowed.has(k));
|
const hits = Object.keys(sl).filter(k => allowed.has(k));
|
||||||
if (hits.length === 0) {
|
if (hits.length === 0) {
|
||||||
|
|||||||
@ -299,7 +299,16 @@ function ScaleLedger({ currentTotals, scaleInputs, setScaleInputs, scalePlug, se
|
|||||||
const hasExcl = excl.rows > 0 && (excl.value !== 0 || excl.units !== 0)
|
const hasExcl = excl.rows > 0 && (excl.value !== 0 || excl.units !== 0)
|
||||||
const onTotal = hasExcl && targetBasis !== 'adjustable' // 'selected total' is the default
|
const onTotal = hasExcl && targetBasis !== 'adjustable' // 'selected total' is the default
|
||||||
const grand = { value: total.value + excl.value, units: total.units + excl.units }
|
const grand = { value: total.value + excl.value, units: total.units + excl.units }
|
||||||
const exclName = (currentTotals?.excludedIters || []).join(' / ') || 'excluded'
|
// Named by the segments themselves where we have them -- "02 - Prior Year"
|
||||||
|
// reads as a thing a forecaster recognises, where "reference" names only the
|
||||||
|
// iter band that happens to exclude it.
|
||||||
|
const exclName = (currentTotals?.excluded?.names || []).join(' · ')
|
||||||
|
|| (currentTotals?.excludedIters || []).join(' / ')
|
||||||
|
|| 'excluded'
|
||||||
|
|
||||||
|
// Everything in the selection is immovable. Worth saying outright: the panel
|
||||||
|
// otherwise prints a row of zeros and leaves the reason to be worked out.
|
||||||
|
const nothingToAdjust = hasExcl && !total.value && !total.units
|
||||||
|
|
||||||
// the basis decides which line the editable rows are measured from
|
// the basis decides which line the editable rows are measured from
|
||||||
const basisOf = (key) => {
|
const basisOf = (key) => {
|
||||||
@ -443,12 +452,15 @@ function ScaleLedger({ currentTotals, scaleInputs, setScaleInputs, scalePlug, se
|
|||||||
{/* Rows the pivot shows but operations cannot write. Listed so the
|
{/* Rows the pivot shows but operations cannot write. Listed so the
|
||||||
panel's figures reconcile with what the grid displays. */}
|
panel's figures reconcile with what the grid displays. */}
|
||||||
{hasExcl && (
|
{hasExcl && (
|
||||||
<tr className="text-gray-600">
|
<tr className="text-amber-700">
|
||||||
<td className="pr-3 whitespace-nowrap">
|
<td className="pr-3 whitespace-nowrap max-w-[16rem] truncate" title={exclName}>
|
||||||
{exclName} <span className="text-gray-500">· fixed</span>
|
{exclName}
|
||||||
|
<span className="ml-1 px-1 py-0.5 rounded bg-amber-50 text-amber-700 text-[10px] uppercase tracking-wide">
|
||||||
|
final
|
||||||
|
</span>
|
||||||
</td>
|
</td>
|
||||||
{measures.map(m => (
|
{measures.map(m => (
|
||||||
<td key={m.key} className={numCell}>
|
<td key={m.key} className={`${numCell} text-amber-700`}>
|
||||||
{m.key === 'price' ? fmtNum(priceOf(excl), m.dp) : fmtNum(excl[m.key], m.dp)}
|
{m.key === 'price' ? fmtNum(priceOf(excl), m.dp) : fmtNum(excl[m.key], m.dp)}
|
||||||
</td>
|
</td>
|
||||||
))}
|
))}
|
||||||
@ -468,6 +480,19 @@ function ScaleLedger({ currentTotals, scaleInputs, setScaleInputs, scalePlug, se
|
|||||||
</tr>
|
</tr>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
|
{/* Zeros in the Adjustable row are a true answer to the wrong question:
|
||||||
|
they say how much can move, not why none of it can. Spell it out
|
||||||
|
where the eye already is, rather than leaving the edit rows to fail
|
||||||
|
silently below. */}
|
||||||
|
{nothingToAdjust && (
|
||||||
|
<tr>
|
||||||
|
<td colSpan={measures.length + 1} className="pt-2 text-amber-700 leading-snug">
|
||||||
|
Nothing in this selection can be adjusted — all of it is {exclName},
|
||||||
|
loaded as {(currentTotals?.excludedIters || []).join(' / ') || 'reference'}.
|
||||||
|
</td>
|
||||||
|
</tr>
|
||||||
|
)}
|
||||||
|
|
||||||
<tr>{rule}{measures.map(m => <td key={m.key} className="p-0 px-2"><div className="border-t border-gray-300 my-1" /></td>)}</tr>
|
<tr>{rule}{measures.map(m => <td key={m.key} className="p-0 px-2"><div className="border-t border-gray-300 my-1" /></td>)}</tr>
|
||||||
|
|
||||||
{/* the edit — three equivalent ways to say the same thing */}
|
{/* the edit — three equivalent ways to say the same thing */}
|
||||||
|
|||||||
@ -12,6 +12,11 @@ import '@perspective-dev/viewer/inline'
|
|||||||
import '@perspective-dev/viewer-datagrid'
|
import '@perspective-dev/viewer-datagrid'
|
||||||
import '@perspective-dev/viewer/themes'
|
import '@perspective-dev/viewer/themes'
|
||||||
|
|
||||||
|
// Slice keys that are not col_meta columns: computed from pf.log when the rows are
|
||||||
|
// served, so they are real columns in the loaded table but have to be resolved back
|
||||||
|
// to log ids server-side. Mirrors COMPUTED_SLICE_COLS in lib/sql_generator.js.
|
||||||
|
const COMPUTED_SLICE_COLS = new Set(['pf_segment', 'pf_bucket'])
|
||||||
|
|
||||||
const LAYOUT_KEY = (vid) => `pf_layout_v${vid}` // last-used layout (auto restore)
|
const LAYOUT_KEY = (vid) => `pf_layout_v${vid}` // last-used layout (auto restore)
|
||||||
const LAYOUTS_KEY = (vid) => `pf_layouts_v${vid}` // named layout list
|
const LAYOUTS_KEY = (vid) => `pf_layouts_v${vid}` // named layout list
|
||||||
|
|
||||||
@ -406,6 +411,13 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
|
|||||||
|
|
||||||
async function totalsFor(sliceObj) {
|
async function totalsFor(sliceObj) {
|
||||||
const filters = [
|
const filters = [
|
||||||
|
// pf_segment and pf_bucket are computed server-side but are ordinary
|
||||||
|
// string columns in the loaded table, so here they filter directly. They
|
||||||
|
// have to be applied, or the ledger totals a wider selection than the
|
||||||
|
// operation will write.
|
||||||
|
...Object.entries(sliceObj)
|
||||||
|
.filter(([col]) => COMPUTED_SLICE_COLS.has(col))
|
||||||
|
.map(([col, val]) => [col, '==', String(val)]),
|
||||||
...Object.entries(sliceObj)
|
...Object.entries(sliceObj)
|
||||||
.filter(([col]) => dimNames.has(col))
|
.filter(([col]) => dimNames.has(col))
|
||||||
.map(([col, val]) => [col, '==', val]),
|
.map(([col, val]) => [col, '==', val]),
|
||||||
@ -424,7 +436,7 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
|
|||||||
// rows the pivot shows but operations cannot write (usually 'reference').
|
// rows the pivot shows but operations cannot write (usually 'reference').
|
||||||
// Kept separate rather than filtered away: the grid total includes them,
|
// Kept separate rather than filtered away: the grid total includes them,
|
||||||
// so the panel has to account for them or the two disagree.
|
// so the panel has to account for them or the two disagree.
|
||||||
const excluded = { value: 0, units: 0, rows: 0 }
|
const excluded = { value: 0, units: 0, rows: 0, names: new Set() }
|
||||||
for (const r of rows) {
|
for (const r of rows) {
|
||||||
const k = r.pf_iter || '?'
|
const k = r.pf_iter || '?'
|
||||||
const val = valueCol ? (parseFloat(r[valueCol]) || 0) : 0
|
const val = valueCol ? (parseFloat(r[valueCol]) || 0) : 0
|
||||||
@ -434,6 +446,10 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
|
|||||||
excluded.value += val
|
excluded.value += val
|
||||||
excluded.units += uni
|
excluded.units += uni
|
||||||
excluded.rows += 1
|
excluded.rows += 1
|
||||||
|
// Name them by what they are, not by the iter band that happens to
|
||||||
|
// exclude them: "02 - Prior Year" means something to a forecaster,
|
||||||
|
// "reference" is the mechanism.
|
||||||
|
if (r.pf_segment) excluded.names.add(String(r.pf_segment))
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -505,10 +521,12 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
|
|||||||
value: acc.value + (ps.excluded?.value || 0),
|
value: acc.value + (ps.excluded?.value || 0),
|
||||||
units: acc.units + (ps.excluded?.units || 0),
|
units: acc.units + (ps.excluded?.units || 0),
|
||||||
rows: acc.rows + (ps.excluded?.rows || 0),
|
rows: acc.rows + (ps.excluded?.rows || 0),
|
||||||
}), { value: 0, units: 0, rows: 0 })
|
names: new Set([...acc.names, ...(ps.excluded?.names || [])]),
|
||||||
|
}), { value: 0, units: 0, rows: 0, names: new Set() })
|
||||||
|
|
||||||
setCurrentTotals({
|
setCurrentTotals({
|
||||||
byIter, byEntry, total, excluded, valueCol, unitsCol, perSlice,
|
byIter, byEntry, total, valueCol, unitsCol, perSlice,
|
||||||
|
excluded: { ...excluded, names: [...excluded.names].sort() },
|
||||||
excludedIters: [...excludeIters],
|
excludedIters: [...excludeIters],
|
||||||
})
|
})
|
||||||
} catch {
|
} catch {
|
||||||
@ -1330,6 +1348,10 @@ export default function Forecast({ sources = [], sourceId, versions = [], versio
|
|||||||
const dateCols = new Set(colMetaRef.current.filter(c => c.role === 'date').map(c => c.cname))
|
const dateCols = new Set(colMetaRef.current.filter(c => c.role === 'date').map(c => c.cname))
|
||||||
const out = {}
|
const out = {}
|
||||||
for (const [k, v] of Object.entries(raw)) {
|
for (const [k, v] of Object.entries(raw)) {
|
||||||
|
// Not col_meta columns, but real ones here and resolvable server-side to the
|
||||||
|
// set of pf.log ids that carry the name. Dropping them is what let a click on
|
||||||
|
// one bucket's cell scale every bucket at that intersection.
|
||||||
|
if (COMPUTED_SLICE_COLS.has(k)) { out[k] = v; continue }
|
||||||
if (dimCols.has(k)) { out[k] = v; continue }
|
if (dimCols.has(k)) { out[k] = v; continue }
|
||||||
if (dateCols.has(k)) {
|
if (dateCols.has(k)) {
|
||||||
const ms = Number(v)
|
const ms = Number(v)
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user