Feature parquet extract module - #126
Conversation
| }; | ||
|
|
||
| datalookup:{[t;symcol;syms;cnt] | ||
| / get lists of indices by file |
There was a problem hiding this comment.
step accumulates (bucketIndex; runningSize) using scan (\). The scan accumulates state across all i correctly, but bins is built as (step\[(0;0);til n])[;0] — indexing column 0 of the scan result. Because step returns a 2-element list (bucketIndex; size), indexing [;0] on the scan result gives the bucket indices only when the scan result is a matrix. However, when n is 1, step\[(0;0);enlist 0] returns a single-element list of a 2-element list, not a 2-column matrix, so [;0] returns the whole inner list (0;sz) rather than just 0. This causes group bins to fail or produce wrong output for a single non-oversized instrument. Fix: use {x[;0]} guarded with a type check, or reshape: bins:(step\[(0;0);til n])[;0] should be first each (step\[(0;0);til n]).
There was a problem hiding this comment.
I've tested this case out of a single non-oversized table and have not encountered the failure anticipated here
q)tabs:t[o[`symcol]];
q)sizes:t`estbyt;
q)n:count tabs;
q)n
1
q)step:{[maxsize;sizes;state;i] sz:sizes i; tot:state 1; $[(tot+sz)>maxsize; (1+state 0; sz); (state 0; tot+sz)]}[maxsize;sizes];
// below returns as normal
q)(step\[(0;0);til n])[;0]
,0
// repeating by explicitly stating enlist 0, again same return
q)step\[(0;0);enlist 0][;0]
,0
q)bins:(step\[(0;0);til n])[;0]
q)value[tabs @ group bins]
DOW
q) plans,:value[tabs @ group bins]
q) plans:(1 + til count plans)!plans;
q)plans
1| DOW
I'm planning to leave this as is, unless there is another reason why I shouldn't keep this.
DIReview Summary3 critical | 5 warning(s) | 0 suggestion(s)
|
| }; | ||
|
|
||
| calcsize:{[tbl;symcol;syms;seqno] | ||
| / find the estimated size in bytes for each instrument per file to be saved down |
There was a problem hiding this comment.
The fix wraps syms in enlist inside the functional where clause: enlist(in;symcol;enlist syms). In a functional select, the in operator requires its right argument to be a list of values to test membership against. If syms is already a list (the typical case — a list of symbols), enlist syms turns it into a nested list (syms;), which will cause a type error at runtime. The original \syms(backtick-name reference) was likely intended to resolve the local variable; ifsymsis already a symbol list, the correct form isenlist(in;symcol;syms)without the extraenlist`.
There was a problem hiding this comment.
Having tested this out, I believe enlist syms is the correct logic here, as in my use cases removing this will throw an error (error looks like the name(s) of instruments).
DIReview Summary0 critical | 2 warning(s) | 0 suggestion(s)
|
DIReview Summary0 critical | 1 warning(s) | 0 suggestion(s)
|
| if[any not key[o] in key[default]; | ||
| .z.m.logerr[`pqx;err:"di.pqx: input keys not recognised - ", "," sv string key[o] where not key[o] in key default]; | ||
| 'err | ||
| ]; |
There was a problem hiding this comment.
The bad-key check reads key[o] (the raw user-supplied overrides), but o may be an empty dict ()!() or even a general dict where key returns a symbol list. That is fine. However, the check any not key[o] in key[default] will throw a type error if o is passed as a non-dict (e.g. a general list), crashing before the user-friendly error message is produced. More importantly, if o is the null/empty dict the any result is 0b and it passes silently — which is correct. The real issue is that key[default] is recomputed on every extract call from the module-level default dict; if default has been extended elsewhere before this check, a key that was not in the original default will not be caught. This is a fragile dependency on the mutable module-level default. Consider capturing key[default] at init time.
| / if one sym per file requested, turn off splitoversized | ||
| if[`onesymperfile in key o; | ||
| if[o`onesymperfile; | ||
| .z.m.loginfo[`pqx;"onesymperfile requested, turning off splitoversized"]; |
There was a problem hiding this comment.
onesymperfile override of splitoversized is applied to opts (the merged dict) only when onesymperfile is present in the raw o dict. If a caller passes onesymperfile as a default by having it set to 1b in default (which it is not currently, but could be changed) and does not pass it in o, the guard if[onesymperfile in key o ] would not fire and splitoversized would never be forced off. The intent is clearly to force it off whenever opts[onesymperfile]is true, regardless of whether it came fromoordefault. The condition should be if[optsonesymperfile; ...] rather than checking key o.
| if[not first manwrite; | ||
| .z.m.logwarn[`pqx;"di.pqx: error writing manifest to disk: ",last manwrite] | ||
| ]; | ||
|
|
There was a problem hiding this comment.
tryfn is called as .z.m.tryfn[set;(hsym $writedir,"manifest";res)]. Looking at tryfn's definition it wraps .[f;x;{...}], so xmust be the argument list forf. setis a binary operator taking(path;data), so the argument list should be passed as a 2-element list. Here (hsym $writedir,"manifest";res) is indeed a 2-element list, which is correct for .[set;(path;data);handler]. However writedir is built elsewhere as a string ending in /, so writedir,"manifest" concatenates to e.g. "pqxout10/pqxtrade/date=2025.07.25/manifest" — a relative path string cast to symbol with hsym `$. If extract is called from a different working directory than the one writedir was constructed relative to, the sidecar will be written to the wrong location while the data files (which use the same writedir) will also be wrong. This is a pre-existing issue with path construction, but the new manifest write makes it observable in a new failure mode.
| ] | ||
| ]; | ||
| plans:flip `seqno`syms!((1 + til count plans);plans); | ||
|
|
There was a problem hiding this comment.
In the new plan function, the onesymperfile branch does plans,:enlist each t[o[symcol]]which extracts the sym-column values from thesymstatstable and enlists each one. This produces a list of singleton sym lists (one symbol per bucket) — matching the shape the packing branch produces. Howeversymstatsat this point still holds the pre-filtered table (it may contain symbols that appear more than once in the sym column ifsymstatswas not de-duped). If two rows share the same symbol, you get two separate buckets for the same symbol, producing two output files for one instrument. Verify thatsymstatsis guaranteed to have exactly one row per symbol before reachingplan`, or add a distinct/dedup here.
DIReview Summary0 critical | 4 warning(s) | 0 suggestion(s)
|
| files:([] file:system"find \"",(1 _ string path),"\" -name \"*.parquet\""); | ||
| files:update split:"/" vs/:file from files; | ||
| lv:1+count where "/"=string path; | ||
| levels:(),datecol,dictcols; |
There was a problem hiding this comment.
The find command is built by string-concatenating 1 _ string path directly into a shell command without any escaping. If hdbdir or tname contains shell metacharacters (spaces, quotes, $, backticks, etc.), this will silently produce wrong results or execute unintended shell commands. Use a safe path construction instead, or at minimum document that path components must not contain shell metacharacters.
| levels:(),datecol,dictcols; | ||
| levelcols:{[datecol;x] (castvirtualcol[datecol;x;];`split)}[datecol] each lv+til count levels; | ||
| files:![files;();0b;(`file,levels)!enlist[({hsym `$x};`file)],levelcols]; | ||
| pqt.mkP (levels#files)!pq.pq each exec file from files |
There was a problem hiding this comment.
lv counts the number of / characters in string path (e.g. string :pqxout1/pqxtrade→ ":pqxout1/pqxtrade"), butfindreturns absolute paths whilepathis a relative hsym. The leading:(stripped by1 _infind) means the split-segment offsets computed from pathdo not match the actual depth of thefindoutput paths, sofiles[;lv+n]will index the wrong path component andcastvirtualcolwill silently reconstruct the wrong values. The level-offset must be computed against the absolute path returned byfind`, not against the module-local relative path symbol.
| / casts the xth hive-style path segment (a "key=value" string) of every row in y's split column to its | ||
| / reconstructed value - a date if it's the datecol level, else a symbol | ||
| :$[datecol = first `$distinct first each "=" vs' v:y[;x]; | ||
| "D"$last each "=" vs' v; |
There was a problem hiding this comment.
castvirtualcol inspects only the first row's key name (first $distinct first each "=" vs' v) to decide whether the level is the datecol. If the directory happens to contain no files (empty filestable),vis empty andfirstreturns null, causing the comparisondatecol = first null` to produce a type error or wrong result. More critically, it assumes all files at that path depth share the same key name — if they don't (mixed directory layouts), only the first file's key name is consulted, silently miscasting the rest.
DIReview Summary0 critical | 3 warning(s) | 0 suggestion(s)
|
2 participants
Converts an in-memory kdb+ table into one or more
.parquetfiles viakx.arrow. Rows are groupedby instrument and packed into files close to a configurable target size, splitting any single
oversized instrument across multiple files where required. A manifest recording what was written
(file, instruments, row count, time range, on-disk size) is accumulated in the module's private
manifesttable