Feature merge - #127
Open
alowrydi wants to merge 6 commits into
Open
Conversation
Rebuilds the scaffolding around the initial draft to match conventions that landed since it was written (di.eodtime, di.depcheck, di.kafka, di.servers, di.heartbeat), while keeping the original merge algorithm (getpartchunks, mergebypart, mergebycol, mergehybrid) - a faithful, line-checked port of TorQ's code/common/merge.q, verified against both real callers (wdb.q, tickerlogreplay.q) - untouched. Bug fix: mergebypart's failure handler double-converted an already-string error message via a redundant string call, which corrupted the log message and made the handler itself throw a second, uncaught error - so a merge failure that should be logged and skipped instead crashed the caller. Fixed and covered by a regression test. The bug predates this refactor (present in the legacy TorQ source too) and was only surfaced by deliberately forcing an upsert failure, since no happy-path test exercised that branch. Two deliberate design decisions: - init now preserves tracked-but-unmerged partition sizes across a re-init (e.g. a live config reload) instead of wiping them - partsizes is orthogonal to the deps a re-init typically changes, and silent data loss is worse than leaving it alone. init logs explicitly when it preserves existing tracked partitions. - mergebycol's column read is intentionally left unprotected (unlike mergebypart's now-guarded upsert): a partial-column failure mid-merge would leave the destination silently inconsistent (some columns updated, others stale), which is worse than failing loudly. Scaffolding changes: - requireinit guard on every exported function except init/getapimeta - normlog/kx.log auto-detection removed; init does strict binary-dict validation requiring info+error only (merge.q never calls warn) - init validation inlined, no setdeps/setconfig split - VERSION file added, version exported, read defensively - deps.q removed - standalone module, no hard deps - getapimeta added for di.api registration - stale sort.csv wording removed from checkpartitiontype's log messages - new: syncpartsizes, giving the receive side of the legacy partsizes IPC fan-out a real, guarded function instead of a raw upsert message - state access normalised to .z.m throughout 148 k4unit assertions, including real on-disk-segment coverage for all three merge paths, the requireinit guard, version/getapimeta shape, syncpartsizes, the re-init preservation behaviour, and the mergebypart error-handler fix.
…etpartchunks drop logging A senior review of the initial push flagged three gaps before this is PR-ready: getfirstcharpartitions had no positive-path test (only the pre-init rejection case), getpartchunks silently dropped untracked partitions with no trace, and merge.md's "two callers" note had gone half-stale now that checkenumerabletype is covered. Adds a positive-path test for getfirstcharpartitions, adds an info-level log line (and test) when getpartchunks drops an untracked partition without changing its filtering behaviour, and updates merge.md accordingly.
…ion, checkenumerabletype A full write-down-and-merge smoke test (real segments, real di.log, both of TorQ's partbyenum and partbyfirstchar write patterns) surfaced three real gaps beyond what the mock-logger k4unit suite could catch: checkenumerabletype built its error message with string[list] instead of ", " sv string list, producing a malformed nested value that crashed under a real logger the moment a non-enumerable parted column was checked - the one case the function exists to catch. Fixed to match checkpartitiontype's already-correct pattern. mergehybrid never actually left the destination `p#-attributed, even when every batch merged cleanly: upsert appends raw values onto an on-disk column without persisting an in-memory attribute, and no single batch/column write can guarantee the whole destination stays grouped once multiple batches (and mergebycol's un-resorted, multi-value segments) have all appended to it. Confirmed empirically both ways: a pure mergebypart merge left no attribute at all, and a partbyfirstchar-style merge through mergebycol left the parted column genuinely unsorted on disk. Fixed with a single, final resort-and-reattribute pass over the whole destination in mergehybrid, after every batch and column has merged - a deliberate, documented departure from the module's "keeps memory flat" goal for that one step, since a destination that's silently never truly parted is worse. mergebypart read every segment in a batch as one unprotected unit, so one missing/corrupt segment file crashed uncaught and took its healthy batch-mates down with it. Now reads each segment individually and protected - a bad segment is error-logged and dropped, the rest of the batch still merges. Adds regression coverage for all three (message-shape assertion, mergehybrid attribute/resort test spanning both merge paths, mergebypart batch-isolation test) and documents the attribute and batch-isolation behaviour in merge.md.
DIReview Summary1 critical | 2 warning(s) | 0 suggestion(s)
|
…irs footgun Wrap mergeonecol's segment-column read so a missing/unreadable column names itself in the propagated error instead of a bare read failure, without changing the deliberate fail-loud behaviour mergebycol relies on. Rename mergehybrid's in-place partdirs reassignment to underlimit so partdirs keeps meaning "everything requested" for future edits. Add regression coverage for the missing-column path and for mergehybrid's overlimit-empty / overlimit-equals-partdirs branches, and document tablename's global-table requirement once for all four functions that share it.
DIReview Summary0 critical | 1 warning(s) | 0 suggestion(s)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On-disk partition-segment merge module, di.merge
Extracts TorQ's
code/common/merge.qinto a standalone kdb-x module - on-disk partition-segmentmerging for the write-down (WDB) flow, called by both
wdb.qandtickerlogreplay.qin legacyTorQ: whole-partition, column-by-column, or a size-driven hybrid of the two, chosen per partition
by a configurable row-count or byte-size limit, plus partition-size tracking to drive that
decision. The last unresolved hard dependency for
di.wdb.Trello ticket - https://trello.com/c/HAJdcl9V/111-kdb-x-merge
Files created
di/merge/init.qmerge.q, defines export of 14 functionsdi/merge/merge.qdi/merge/merge.mddi/merge/test.csvdi/merge/VERSIONHow to test
190/190 assertions passing.
Coverage includes: dependency validation, the
requireinitguard on every exported function,config application (row-count vs byte-size batching,
partlimitsplitting), partition-sizetracking and cross-process sync, both re-init and failure-isolation design decisions with
regression coverage,
version/getapimetashape, and end-to-endmergebypart/mergebycol/mergehybridagainst real on-disk segments.Design decisions
1. No hard module dependencies; parted columns supplied by the caller - Legacy
merge.qread atable's parted column(s) from
.sort.params(populated fromsort.csv) viagetextrapartitiontype. That coupling was removed on extraction: the parted column(s)(
extrapartitiontype) are passed in by the caller instead, sodi.mergedoesn't depend ondi.sort- confirmed against the plan's own "Hard dependency tree", which listsdi.mergeunderSTANDALONE. Only
logis injected, viainit.2.
syncpartsizes- a real receive-side API for a legacy raw-IPC pattern - Legacywdb.qfanspartition-size state out to sort-worker processes over raw async IPC with no symmetric
receive-side function: a receiving process evaluated the raw
(upsert;.merge.partsizes;y)tuple directly, which only worked if it had already loadedmerge.qso the table existed with the right schema - an undocumented, load-order-dependent contract.syncpartsizes[t]gives the receive side a real,requireinit`-guarded function to go through instead.3.
initpreserves tracked-but-unmerged partition sizes across a re-init - Callinginitagainwith valid deps (e.g. a live config reload) does not wipe
.z.m.partsizes; segments tracked sincethe last
clearpartsizes[]survive.partsizesis orthogonal to thelog/mergebybytelimit/partlimitdeps a re-init is typically changing, and silently discarding tracked-but-unmergedsegment sizes is a worse failure mode than leaving them alone.
initlogs explicitly when itpreserves state, so the decision is visible rather than something a future debugger has to
discover by reading source.
4.
mergebypartisolates each segment's read;mergebycoldeliberately does not protect itscolumn read - Not equivalent failure modes, so making them match wouldn't obviously be the safer
choice.
mergebypartnow reads each segment in a batch individually and protected - amissing/corrupt segment is error-logged and dropped without disturbing its batch-mates, which
still merge.
mergebycolmerges one column at a time into the same destination; a swallowedfailure partway through would leave some columns reflecting the new data and others silently
stale - a genuinely worse, silently-inconsistent partition, not just a delayed merge. So
mergebycol's column read stays intentionally unprotected.5. The parted attribute is applied once, to the whole destination, only via
mergehybrid- Afull write-down-and-merge smoke test against real segments and real
di.log(not mocks) foundthat neither
mergebypartnormergebycolalone can guaranteedestends up with the`p#attribute genuinely set:
upsertappends raw values onto an on-disk column without persisting anin-memory attribute, and no single batch/column write can guarantee the whole destination stays
grouped once several have all appended to it.
mergehybridcloses this with one finalread-resort-reattribute-rewrite pass over the complete destination, after every batch and column
has merged - a deliberate, documented departure from the module's memory-flat design goal for that
one step, since a destination that's silently never truly parted is worse.
mergebypart/mergebycolcalled standalone, bypassingmergehybrid, do not get this guarantee automatically -documented in
merge.md;di.wdb(in progress) will always route throughmergehybridfor thisreason.
6. A real-logger smoke test, not just the k4unit mock suite - Mock loggers only confirm a
message was logged at the right level; they don't process message content, so they can't catch a
malformed message - a list where a flat string was expected, for instance. That gap is exactly
what a real-logger pass found:
checkenumerabletypebuilt its error message withstringapplieddirectly to a symbol list rather than
", " sv string ..., producing a malformed nested valuethat crashed the moment a non-enumerable parted column was checked - the one case the function
exists to catch. Fixed to match
checkpartitiontype's already-correct pattern, with a regressiontest that asserts on the message's structure (
10h=type), not just that it fired.Checklist
consistency.mdandstyle.mdmerge.mddocuments all exported functions, config, therequireinitguard, all designdecisions, cross-process partition-size sync, and a usage example
di.*modules - standaloneDocumentation
See
merge.mdfor full reference including the dependency contract, config keys, exportedfunction documentation, the
requireinitguard, all six design decisions, cross-processpartition-size sync, and a usage example.