Feature dbwrite#95
Conversation
|
…ull guard - Export savedownmanipulation so consumers can register per-table pre-write functions - loadconfig warns and loads defaultparams when called with a null file rather than silently failing - sort falls back to defaultparams directly; removes defaultfile indirection from init.q - Tests: add savedownmanipulation registered function and error recovery cases, loadconfig null case, move all on-disk cleanup to after blocks - Docs updated throughout to match Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nput crash - loadconfig: explicit symbol type check with clear error before null check - applyattr: guard against invalid att (must be in `p`s`g`u), null colname guard - sort: type check for d (must be symbol or list); empty list returns () safely - test.csv: 78 tests covering wrong types, nulls, empty inputs across all functions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ng test setup Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| .z.m.loginfo[`dbwrite;"garbage collection returned ",(string `long$r%1048576),"MB. ",.z.M.memstats[]] | ||
| }; | ||
|
|
||
| init:{[config;deps] |
There was a problem hiding this comment.
config argument not actually called within this function?
|
You need to think about this a bit more and apply some judgement. When we are stripping code out of TorQ we are trying to créate a set of standalone, isolated modules. What is the purpose of this module? It is to provide a set of utilities for writing tables, including sorting them on disk. Postreplay and savedownmanipulation likely don’t have relevance in here (the are artifacts of other parts of TorQ). Can you also coordinate with Alex because there’s overlap with the sort lib, I think the sort lib should live in here. |
- Add savedown[dir;part;tabname;data]: enumerates syms, applies p#sym, writes partition via .Q.par + set, then calls sort - Add upsert[dir;part;tabname;data]: appends to existing partition via functional amend, then re-sorts - Remove manipulate, savedownmanipulation, postreplay (TorQ-influenced) - Update export dict, init comment, tests, and docs accordingly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename upsert to appenddown (upsert is a reserved word in kdb+) - Remove auto-sort from appenddown; caller sorts explicitly when done - Fix savedown/appenddown to use trailing-slash path via ` sv (.Q.par[...];`) so xasc treats it as a splayed table directory (without slash, set writes a binary file and xasc fails with "Not a directory") - Add existence guard to appenddown: throws if partition path does not exist - Update init.q export dict, tests, and docs accordingly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… p attribute before sorting resulting in a ufail
- readcsv now stores parsed config in .z.m.sortconfig instead of returning it - add setconfig to allow consumers to provide an in-memory config table directly - sort and savedown drop their config parameter; both read .z.m.sortconfig, falling back to defaultparams - gc is called at the end of savedown; removed from exports as it is not dbwrite-specific - init initialises .z.m.sortconfig to (::) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- add getconfig export to allow consumers to inspect .z.m.sortconfig - readcsv now calls checkconfig after parsecsv so invalid att values are caught at load time - tests updated throughout: readcsv/sort/savedown call sites use new signatures; config validation tests moved to setconfig section; gc section removed; setconfig happy-path and validation tests added (159 tests, all passing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- remove stateless config-per-call design description; module now stores config in .z.m.sortconfig - document readcsv as a store operation (not a return); add setconfig and getconfig sections - update sort and savedown signatures (config arg removed from both) - note gc is called automatically by savedown; remove gc from exported symbols list - update test suite description to match new setconfig/getconfig coverage Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…plyattr docs - readcsv now accepts string paths (10h) by converting via hsym `$, avoiding the manual hsym `$"..." dance; bare symbols still coerced as before - fix malformed test csv: string path test line had unescaped double quotes causing '/ parse error when k4unit loaded the suite - add three tests covering string path happy path - add type comment to applyattr (dloc/colname/att were undocumented) - clarify sort comment: explicit that att assignments from config are applied Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
As it stands, the sort function operates on one table & partition combo. A list of partitions can be passed. / end-of-day, one partition Considering adding the following use case functions. Not sure if they would get used/be useful.
Also probably a style choice, do we want the modules to be lightweight and functional or bulkier with a bit more usecase coverage?? |
| }; | ||
|
|
||
| / internal - validate the header and data rows, then parse csv lines into a config table | ||
| parsecsv:{[lines] |
There was a problem hiding this comment.
I think AI is overthinking things here - it has decided 0: csv reading isn't fit for purpose and decided to rebuild it from first principles. But I think reinventing basic q functionality sets a bad precedent if we give claude free reign, we'll end up with a lot of bloated code where AI is solving problems that don't actually exist in practice. I think this function should be much shorter - just process the lines using 0:, and leave the validation to checkconfig
There was a problem hiding this comment.
Agree with this,
I have moved certain validation checks from parsecsv into the checkconfig function and also updated the file reading to use 0:
| }; | ||
|
|
||
| / internal - validate a sort-config table, signalling a clear error if it is malformed | ||
| checkconfig:{[t] |
There was a problem hiding this comment.
The errors here - should they also be printed through .z.m.log[error] , like the readerr one above? If not why not- I think we should be consistent unless there is a reason I'm missing. If the log library returns the string as well as printing it (I'm not sure it does - do check), then this can be a one liner, log and throw the result:
'.z.m.log[error] "di.dbwrite: ..." If the log function doesn't return anything then it would have to be: .z.m.log[error] errstr:"di.dbwrite: ..."; ' errstr;
There was a problem hiding this comment.
When updating the logging to align with the newer standards we have set this issue had already been addressed.
Change already made so no changes required off the back of this.
| }; | ||
|
|
||
| / internal - resolve which config rows apply to a table | ||
| getsortparams:{[config;tab;st] |
There was a problem hiding this comment.
I don't like 'st' being a parameter to this function, given it is just the 'tab' name duplicated as a string. Just create st as a local variable in here again if required. There is no performance consideration here - to be honest I would likely just use string[tab] everywhere st is used, but that is a stylistic choice. However function parameters should only be added when they are strictly necessary, this one isn't since it is derivable from another param
There was a problem hiding this comment.
Interesting point, makes a lot of sense. Have updated the getsortparams function following this.
DIReview Summary3 critical | 6 warning(s) | 0 suggestion(s)
|
| / matter; cast sort to boolean if present. structural validation is left to checkconfig. | ||
| t:((count "," vs first lines)#"S";enlist",") 0: lines; | ||
| :$[`sort in cols t; update sort:"B"$string sort from t; t]; | ||
| }; |
There was a problem hiding this comment.
When lines contains only a header row (zero data rows), first lines is the header string, but "B"$string sort in the subsequent update will be applied to a column of type S (symbols cast from the empty sort column). For the zero-data-row case this is likely harmless, but the real problem is (count "," vs first lines)#"S" — using a uniform "S" type vector for all columns means the sort column is loaded as symbols (e.g. `1, `0), not as booleans or even strings. "B"$string sort then converts `1→"1"→1b and `0→"0"→0b, which works for clean data, but `x→"x"→0b silently coerces instead of being rejected — contradicting the stated intent. More critically, any att value that is a valid symbol (`, `p, etc.) will be loaded correctly, but tabname/column values that happen to look like numbers (e.g. 1, 0) will be interned as symbols without issue, so actual data corruption for those is not a concern. The real defect is silent coercion of invalid sort values: a non-0/1 sort entry becomes 0b without error. The doc says this is intentional, but the checkconfig validator must then reject 0b-coerced rows from a value like "x" — if it does not check the original string, these pass silently. Fix by loading sort as "C" (character/string) and validating before casting, or keep the all-"S" load but add an explicit pre-cast validity check in checkconfig for the original symbol values.
There was a problem hiding this comment.
Intentional and documented. We chose to use 0: and checkconfig rather than manual parsing. This is the same thing Jamie was talking about earlier "AI is solving problems that don't actually exist in practice".
No changes needed.
DIReview Summary1 critical | 0 warning(s) | 0 suggestion(s)
|
6 participants
DBwrite module code, including test.csv suite. Init.q and dbwrite.q code to define logic and export it. dbwrite.md as a README.