Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Parquet → CSV converter to the converters subsystem, wiring it into the central converter registry and introducing dependencies needed for Parquet parsing and CSV output.
Changes:
- Register the new
parquetconverter insrc/converters/main.ts. - Implement Parquet row-group reading and CSV streaming in
src/converters/parquet.tsusinghyparquet+csv-stringify. - Add a basic Bun test for the parquet converter and update dependencies/lockfile.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/converters/parquet.test.ts | Adds initial tests for the new converter (currently minimal and has an async assertion issue). |
| src/converters/parquet.ts | New Parquet→CSV converter implementation using hyparquet metadata + row-group reads and CSV stringification. |
| src/converters/main.ts | Registers the parquet converter so it can be selected/auto-matched by the main conversion flow. |
| package.json | Adds csv-stringify, hyparquet, and (currently unused) duckdb. |
| bun.lock | Lockfile updates for the new dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/converters/parquet.ts">
<violation number="1" location="src/converters/parquet.ts:46">
P1: The `finish` and `error` event handlers are `async` and `await fileHandle.close()`. If `close()` rejects, the thrown error prevents `resolve`/`reject` from being called, leaving the outer Promise permanently pending and producing an unhandled rejection. Use non-async handlers that always settle the outer promise, e.g. `fileHandle.close().catch(() => {}).finally(() => resolve(...))`.</violation>
<violation number="2" location="src/converters/parquet.ts:74">
P1: Row writes ignore stream backpressure, which can cause excessive buffering/memory growth on large parquet files.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:24">
P2: `duckdb` is added as a dependency but is not imported or used anywhere in the codebase. Since it's a large native dependency with a native build step (`node-gyp`), it should be removed to avoid unnecessary install time, binary size, and potential build failures.</violation>
</file>
<file name="tests/converters/parquet.test.ts">
<violation number="1" location="tests/converters/parquet.test.ts:67">
P2: Rejection assertion is not awaited/returned, so the async failure-path test may pass without validating the expected rejection.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/converters/parquet.ts">
<violation number="1" location="src/converters/parquet.ts:118">
P2: On conversion errors, the output write stream is not explicitly closed/destroyed, which can leave the destination file descriptor open or partial output file lingering.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
and got: |
|
@C4illin — the write after end has been fixed. The root cause was that hyparquet's parquetRead calls the onComplete callback without await, so CSV writes could still be pending when stringifier.end() was called. I also took the opportunity to add support for additional compression codecs via hyparquet-compressors, so the converter now handles ZSTD, GZIP, BROTLI, and LZ4. |
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/converters/parquet.test.ts">
<violation number="1" location="tests/converters/parquet.test.ts:28">
P2: The success test discards every output chunk and checks only `"Done"`, so it passes even when conversion emits no CSV or incorrect headers and rows. Capture the written chunks and assert the expected CSV content.</violation>
</file>
<file name="src/converters/parquet.ts">
<violation number="1" location="src/converters/parquet.ts:37">
P2: CSV cells are written verbatim, so parquet values starting with `=`, `+`, `-`, or `@` become live formulas when the CSV is opened in Excel/Sheets (CSV formula injection). Since the input parquet is user-supplied, this lets one user embed hyperlink/formula payloads in another user's downloaded file. csv-stringify 6.x supports `escape_formulas: true`, which prefixes such values to make them inert — enable it.</violation>
<violation number="2" location="src/converters/parquet.ts:61">
P3: When the write stream fails, `settle(err)` rejects the promise but nothing aborts the row-processing loop: it keeps calling `parquetRead` for every remaining row group and writing into a stringifier whose destination is dead. Writes then block forever on a `drain` that never fires, so the `Promise.all(writePromises)` chain and IIFE hang until process exit, doing wasted decode work. Cancel the write loop when any stream errors — e.g. destroy `stringifier`/set an aborted flag checked by `writeRow` before writing — instead of leaving the loop to drain. (The catch block already destroys both streams for errors thrown inside the loop; the stream 'error' path is the one missing this.)</violation>
<violation number="3" location="src/converters/parquet.ts:67">
P2: Parquet INT64/UINT64 columns are decoded by hyparquet as bigint, and this converts them to Number. Values above 2^53 (large IDs, u64 hashes) lose precision silently, corrupting the CSV output, which matters for a converter whose job is data fidelity. Emit bigints as strings (`String(value)` / `value.toString()`) so the CSV preserves the exact value. Apply the same change to the JSON replacer two lines above, which uses `Number(val)` for nested bigints.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Re-trigger cubic
| const mockCreateWriteStream = mock(() => { | ||
| return new Writable({ | ||
| write(_chunk, _encoding, callback) { | ||
| callback(); |
There was a problem hiding this comment.
P2: The success test discards every output chunk and checks only "Done", so it passes even when conversion emits no CSV or incorrect headers and rows. Capture the written chunks and assert the expected CSV content.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At tests/converters/parquet.test.ts, line 28:
<comment>The success test discards every output chunk and checks only `"Done"`, so it passes even when conversion emits no CSV or incorrect headers and rows. Capture the written chunks and assert the expected CSV content.</comment>
<file context>
@@ -0,0 +1,81 @@
+const mockCreateWriteStream = mock(() => {
+ return new Writable({
+ write(_chunk, _encoding, callback) {
+ callback();
+ },
+ });
</file context>
| }; | ||
|
|
||
| const metadata = await parquetMetadataAsync(file, { compressors } as never); | ||
| const stringifier = stringify({ |
There was a problem hiding this comment.
P2: CSV cells are written verbatim, so parquet values starting with =, +, -, or @ become live formulas when the CSV is opened in Excel/Sheets (CSV formula injection). Since the input parquet is user-supplied, this lets one user embed hyperlink/formula payloads in another user's downloaded file. csv-stringify 6.x supports escape_formulas: true, which prefixes such values to make them inert — enable it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/converters/parquet.ts, line 37:
<comment>CSV cells are written verbatim, so parquet values starting with `=`, `+`, `-`, or `@` become live formulas when the CSV is opened in Excel/Sheets (CSV formula injection). Since the input parquet is user-supplied, this lets one user embed hyperlink/formula payloads in another user's downloaded file. csv-stringify 6.x supports `escape_formulas: true`, which prefixes such values to make them inert — enable it.</comment>
<file context>
@@ -0,0 +1,118 @@
+ };
+
+ const metadata = await parquetMetadataAsync(file, { compressors } as never);
+ const stringifier = stringify({
+ header: true,
+ cast: {
</file context>
| const stringifier = stringify({ | |
| const stringifier = stringify({ | |
| header: true, | |
| escape_formulas: true, |
| const writeRow = async (rawRow: Record<string, unknown>) => { | ||
| const row: Record<string, unknown> = {}; | ||
| for (const [key, value] of Object.entries(rawRow)) { | ||
| row[key] = typeof value === "bigint" ? Number(value) : value; |
There was a problem hiding this comment.
P2: Parquet INT64/UINT64 columns are decoded by hyparquet as bigint, and this converts them to Number. Values above 2^53 (large IDs, u64 hashes) lose precision silently, corrupting the CSV output, which matters for a converter whose job is data fidelity. Emit bigints as strings (String(value) / value.toString()) so the CSV preserves the exact value. Apply the same change to the JSON replacer two lines above, which uses Number(val) for nested bigints.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/converters/parquet.ts, line 67:
<comment>Parquet INT64/UINT64 columns are decoded by hyparquet as bigint, and this converts them to Number. Values above 2^53 (large IDs, u64 hashes) lose precision silently, corrupting the CSV output, which matters for a converter whose job is data fidelity. Emit bigints as strings (`String(value)` / `value.toString()`) so the CSV preserves the exact value. Apply the same change to the JSON replacer two lines above, which uses `Number(val)` for nested bigints.</comment>
<file context>
@@ -0,0 +1,118 @@
+ const writeRow = async (rawRow: Record<string, unknown>) => {
+ const row: Record<string, unknown> = {};
+ for (const [key, value] of Object.entries(rawRow)) {
+ row[key] = typeof value === "bigint" ? Number(value) : value;
+ }
+ if (stringifier.write(row)) return;
</file context>
| row[key] = typeof value === "bigint" ? Number(value) : value; | |
| row[key] = typeof value === "bigint" ? String(value) : value; |
| }; | ||
|
|
||
| writeStream.on("finish", settle); | ||
| writeStream.on("error", settle); |
There was a problem hiding this comment.
P3: When the write stream fails, settle(err) rejects the promise but nothing aborts the row-processing loop: it keeps calling parquetRead for every remaining row group and writing into a stringifier whose destination is dead. Writes then block forever on a drain that never fires, so the Promise.all(writePromises) chain and IIFE hang until process exit, doing wasted decode work. Cancel the write loop when any stream errors — e.g. destroy stringifier/set an aborted flag checked by writeRow before writing — instead of leaving the loop to drain. (The catch block already destroys both streams for errors thrown inside the loop; the stream 'error' path is the one missing this.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At src/converters/parquet.ts, line 61:
<comment>When the write stream fails, `settle(err)` rejects the promise but nothing aborts the row-processing loop: it keeps calling `parquetRead` for every remaining row group and writing into a stringifier whose destination is dead. Writes then block forever on a `drain` that never fires, so the `Promise.all(writePromises)` chain and IIFE hang until process exit, doing wasted decode work. Cancel the write loop when any stream errors — e.g. destroy `stringifier`/set an aborted flag checked by `writeRow` before writing — instead of leaving the loop to drain. (The catch block already destroys both streams for errors thrown inside the loop; the stream 'error' path is the one missing this.)</comment>
<file context>
@@ -0,0 +1,118 @@
+ };
+
+ writeStream.on("finish", settle);
+ writeStream.on("error", settle);
+ stringifier.on("error", settle);
+
</file context>
This change introduces support for converting Parquet files to CSV using hyparquet for better version 2 support and memory-efficient streaming. It includes:
Summary by cubic
Adds Parquet→CSV conversion using
hyparquet, streaming viacsv-stringifywith compressor support fromhyparquet-compressors.parquetconverter streams row groups to CSV with headers, supports Snappy/Zstd, handles backpressure with reliable error cleanup; tests cover success plus read/metadata failures.Written for commit 004ee00. Summary will update on new commits.