Skip to content

fix(query): support update over parted tables and prevent column duplication by: - #406

Merged
singaraiona merged 5 commits into
RayforceDB:devfrom
belowzeroff:fix/update-parted-columns
Aug 16, 2026
Merged

fix(query): support update over parted tables and prevent column duplication by:#406
singaraiona merged 5 commits into
RayforceDB:devfrom
belowzeroff:fix/update-parted-columns

Conversation

@belowzeroff

Copy link
Copy Markdown
Contributor

What a user sees today

Two distinct update failures hit tables loaded from a partitioned database with .db.parted.get — and one hits any table using update with by:.

1. Updating an existing column of a parted table crashes

(set trades (.db.parted.get "/data/market" 'trades))
(update {v: (+ v 100) from: trades})          ;; error
(update {v: 5 from: trades})                  ;; error
(update {v: 99 from: trades where: (> k 1)})  ;; error

Every modifier of an existing column aborts the script:

  • plain / scalar broadcast →
    error: type: update: expression type I64 does not match ? column
  • where:-masked write →
    error: type: vec_new: type must be a positive concrete vector type, got ?
  • grouped by: write →
    error: type: group: argument must be a vector or list, got ?

Adding a new column ({new_col: ...}) worked, so the failure looked
inconsistent and was hard to diagnose. Root cause: parted data columns carry
the RAY_PARTED_BASE wrapper type (printed as ?) plus a MAPCOMMON partition
key; the update pipeline read them through ray_vec_new / ray_data / the
per-group gather, none of which understands the parted/segmented shape.
select already handled this by materialising parted columns — update now
flattens the input table once instead of failing.

2. update ... by: silently does nothing to the target column

(update {w: (sum v) from: T by: k})   ;; T already has column w

The query "succeeds" (no error) but appears to produce no change: the
aggregate is appended as a duplicate column, so the schema becomes
[k v w w], (key T) suddenly lists the column twice, and (at T 'w)
keeps returning the stale original values. Affects flat tables too —
not just parted ones. Root cause: ray_table_add_col always appends, and
the by: branch never skipped the source columns that the update dict
replaces.

Fix

  • ray_update flattens a parted input once through
    query_materialize_parted_col (mirroring select), so every branch below
    sees wrapper-free vectors.
  • The by: branch skips source columns that the update dict replaces when
    copying the initial schema, so the aggregate lands as the single (correct)
    target column. Aggregates now broadcast to every row of their group (kdb
    by: semantics).

Tests

  • New test/rfl/query/update_parted.rfl — regression for the parted cases
    (modify / scalar / where / by: / mixed add+modify / in-place) with flat
    oracles.
  • test/rfl/query/query_update_coverage.rfl — corrected by: expectations
    (the old test passed for the wrong reason: it read the duplicate original
    column).

Full suite: ./rayforce.test passes (exit 0).

belowzeroff and others added 2 commits August 15, 2026 09:02
…ication by:

Two bugs in ray_update hit parted (and, for the second one, flat) tables:

1. MODIFYING an existing column of a parted table failed because the
   update path read the RAY_PARTED_BASE wrapper type / MAPCOMMON segment
   shape through ray_vec_new, ray_data and the per-group gather, none of
   which understands a parted column (error: 'expression type I64 does
   not match ? column', by: path: 'group: argument must be a vector').
   Flatten the whole input table once, the way select does.

2. A by: update on an EXISTING target column appended the aggregate as a
   second column with the same name instead of replacing the original,
   so the schema became [k v w w] and 'at' kept reading the stale value.
   ray_table_add_col always appends, so skip source columns that the
   update dict replaces when copying the initial schema.

Adds regression coverage for both in update_parted.rfl and corrects the
by: broadcast expectations in query_update_coverage (the aggregate now
lands on every row of its group, kdb style).

@singaraiona singaraiona left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The parted-table materialization is a sensible reuse of the existing helper, and the focused cases look good. I found three issues to address before merge:

  1. test/rfl/query/update_parted.rfl:93 calls (exit 0). rayforce.test evaluates this file inside the test-runner process, so this terminates the entire runner before it records this test or executes later tests. A filtered run prints only the test name and exits successfully; removing the line produces the normal PASS summary. This can mask later failures while leaving CI green.

  2. src/ops/query.c:11412-11437 skips replaced columns and appends them later. That removes duplicate names, but it also changes schema order: updating v in [k v w] produces [k w v], and updating k produces [v w k]. Existing targets should be substituted in their original slots; only genuinely new columns should be appended. Please add a regression where the updated target is not last.

  3. Empty grouped updates lose the target type. For an empty table with k:I64, v:F64, w:F64, (update {w: (sum v) from: E by: k}) returns w:I64. With no groups, first_group never retypes the provisional I64 output. Please preserve or infer the type for the zero-group case and add a regression.

@singaraiona singaraiona left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — both bugs are real (verified A/B against current dev: the [k v w w] duplication with stale reads, and the parted-update type errors), the new tests genuinely fail on base, the branch merges cleanly onto current dev, and the full suite is green on the merged result. Two things before this can land:

1. Blocker — vector-valued by: expressions now zero out an existing column (new data-loss regression). The broadcast loop only handles ray_is_atom(agg_result); a per-row expression writes nothing, and the new memset leaves the replaced column all zeros:

(set T (table [k v w] (list [1 2 1 2] [10 20 30 40] [7 7 7 7])))
(at (update {v: (* v 2) from: T by: k}) 'v)
;; base: [10 20 30 40]  (unchanged — the duplicate-column bug masked it)
;; PR:   [0 0 0 0]      (column destroyed)

update v:2*v by k from t is valid kdb ([20 40 60 80]). Please add a ray_is_vec(agg_result) && ray_len(agg_result) == gsize branch that scatters elementwise through idxs[r] (symmetric with the atom branch) — or reject vector results with a loud nyi error. Either way, the query_update_coverage.rfl expectation of [0 0 0 0] must not merge as-is. (Do keep the memset itself — base's recycled-buffer garbage there was a real latent bug.)

2. Docs — the parted contract change needs a sentence. The flatten turns a loud error into a quiet, non-persisted, fully-materialized mutation: the result is in-memory only, re-reading the store shows the original values, and the table loses its parted/mmap identity. The direction is fine (docs/docs/storage/index.md already promises update works on parted tables), but please add a line there and/or in docs/docs/namespaces/db.md stating that update over a parted table materializes the whole table and does not write back to the store.

Minor, non-blocking: duplicate dict keys still append ({v: (sum v) v: (max v)}[k v v]); and the ngroups == 0 path sets out_col->len without the zero-fill the other allocation gets — currently unreachable, but cheap to make uniform.

An `update {col: <expr> by: k}` whose per-group expression returns a vector
(valid kdb, e.g. `update v: 2*v by k`) wrote nothing: the broadcast loop
only handled atom results, so the freshly memset-zeroed output column was
left all-zero — silent data loss that the earlier duplicate-column bug had
masked. Add a scatter branch symmetric with the atom broadcast: when the
result is a vector of the group's size, store element r at idxs[r]. A vector
whose length is neither 1 nor the group size has no row-aligned meaning, so
reject it with a loud `length` error rather than leave zeros in place.

Also zero-fill the ngroups==0 output column to match the per-group path
(uniform, no allocator garbage), and document in the storage guide that
`update` over a partitioned table materializes the whole table in memory
and does not write back to the store.
@belowzeroff

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough review — addressed in ba4dfb63 (rebased/merged onto current dev).

1. Blocker — vector-valued by: zeroing an existing column. Fixed. The broadcast now has a scatter branch symmetric with the atom one: when a group's result is a vector of the group's size, element r is stored at idxs[r]. Verified on your exact repro:

(set T (table [k v w] (list [1 2 1 2] [10 20 30 40] [7 7 7 7])))
(at (update {v: (* v 2) from: T by: k}) 'v)   ;; => [20 40 60 80]
(at (update {v: (* v 2) from: T by: k}) 'w)   ;; => [7 7 7 7]  (untouched)

The query_update_coverage.rfl expectation is now [20 40 60 80] (not [0 0 0 0]), plus a NEW-column case, the replace-in-place case, and a length-mismatch case. A vector whose length is neither 1 nor the group size has no row-aligned meaning, so it's rejected with a loud length error rather than left as zeros. The memset stays (it's what makes the latent garbage bug go away).

2. Docs. Added a warning to docs/docs/storage/index.md: update over a partitioned table materializes the whole table in memory, is not written back to the store, and the result loses its parted/mmap identity.

3. Minor — ngroups == 0 zero-fill. Made uniform with the per-group path (memset after setting len).

Minor — duplicate dict keys ({v: (sum v) v: (max v)}[k v v]). Left as-is for now: it predates this PR and the right resolution (last-wins vs. error) feels like a separate call — happy to fold in whichever you prefer.

make test: 3689/3690 (1 skipped, 0 failed), no sanitizer diagnostics.

@singaraiona
singaraiona merged commit 91fe7e3 into RayforceDB:dev Aug 16, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants