Skip to content

Feature parquet extract module - #126

Open
ConorSwainDI wants to merge 12 commits into
mainfrom
parquetExtractModule
Open

Feature parquet extract module#126
ConorSwainDI wants to merge 12 commits into
mainfrom
parquetExtractModule

Conversation

@ConorSwainDI

Copy link
Copy Markdown

Converts an in-memory kdb+ table into one or more .parquet files via kx.arrow. Rows are grouped
by 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
manifest table

Comment thread di/pqx/pqx.q
Comment thread di/pqx/pqx.q
Comment thread di/pqx/pqx.q
Comment thread di/pqx/pqx.q
Comment thread di/pqx/pqx.q
};

datalookup:{[t;symcol;syms;cnt]
/ get lists of indices by file

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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]).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread di/pqx/pqx.q
Comment thread di/pqx/pqx.q Outdated
Comment thread di/pqx/pqx.q
@DI-Software-Engineering

Copy link
Copy Markdown

DIReview Summary

3 critical | 5 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

Comment thread di/pqx/pqx.q Outdated
Comment thread di/pqx/pqx.q
};

calcsize:{[tbl;symcol;syms;seqno]
/ find the estimated size in bytes for each instrument per file to be saved down

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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`.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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).

@DI-Software-Engineering

Copy link
Copy Markdown

DIReview Summary

0 critical | 2 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

Comment thread di/pqx/pqx.q
@DI-Software-Engineering

Copy link
Copy Markdown

DIReview Summary

0 critical | 1 warning(s) | 0 suggestion(s)

⚠️ Spec check skipped — tracker lookup failed (NO_REF_FOUND). Standards axis only.

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