Skip to content

fix(node): return an error instead of panicking on a zero-field JSON schema - #3112

Draft
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-xphg86-json-arrow-empty-schema
Draft

fix(node): return an error instead of panicking on a zero-field JSON schema#3112
phil-opp wants to merge 1 commit into
mainfrom
claude/dreamy-bardeen-xphg86-json-arrow-empty-schema

Conversation

@phil-opp

Copy link
Copy Markdown
Collaborator

Issue

read_json_value_as_arrow (apis/rust/node/src/daemon_connection/json_to_arrow.rs) ended with:

Ok(batch.column(0).to_data())

RecordBatch::column(0) indexes out of bounds and panics when the batch has no columns. This is a pub fn whose schema is caller-supplied, and a zero-field schema is reachable: the node integration-testing harness (node_integration_testing.rs) builds the schema via infer_json_schema_from_iterator(...) when no data_type is given, and an empty JSON array infers a schema with no fields. That degenerate-but-valid input turns into a panic instead of a recoverable error. (The sibling read_from_json_with_schema is unaffected — its wrapped() always injects an "inner" field, guaranteeing ≥1 column.)

Fix

Use batch.columns().first().context(...)? so a zero-column batch returns an eyre::Result error rather than panicking:

Ok(batch
    .columns()
    .first()
    .context("JSON record batch has no columns")?
    .to_data())

Validation

cargo test -p dora-node-api --lib json_to_arrow    # 2 passed (new)
cargo clippy -p dora-node-api -- -D warnings
cargo fmt --all -- --check

New unit tests cover: a zero-field schema returns Err (no panic), and a normal single-field schema still round-trips.


⚠️ This is a machine-generated PR authored by Claude (Claude Code) as part of an automated codebase review. The finding was verified by hand against origin/main before opening. Please review carefully before merging.

🤖 Generated with Claude Code


Generated by Claude Code

@trunk-io

trunk-io Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Merging to main in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@phil-opp
phil-opp force-pushed the claude/dreamy-bardeen-xphg86-json-arrow-empty-schema branch from e8f4d6a to 6496683 Compare August 10, 2026 23:56
…schema

Both JSON->Arrow readers finished with `batch.column(0).to_data()`.
`RecordBatch::column(0)` indexes out of bounds and panics when the batch
has no columns.

In `read_json_value_as_arrow` the `schema` is caller-supplied — e.g.
inferred from an empty JSON array via `infer_json_schema_from_iterator`
in the node integration-testing harness — so a degenerate zero-field
schema is reachable and turns a bad input into a panic. The sibling
`read_from_json_with_schema` always sees the `"inner"` field injected by
`wrapped()` (so it has a column in practice), but is guarded the same way
for consistency and defense in depth.

Extract a shared `first_column_data` helper that returns an `eyre::Result`
error for a zero-column batch, and call it from both readers. Two unit
tests cover the zero-field (Err, no panic) and normal single-field
(round-trip) cases.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PYiYgEo2WKBXfGooqP2WG5
@phil-opp
phil-opp force-pushed the claude/dreamy-bardeen-xphg86-json-arrow-empty-schema branch from 6496683 to 3752e55 Compare August 11, 2026 00:13

Copy link
Copy Markdown
Collaborator Author

🤖 Automated review by Claude — this is a fully automated review with no human in the loop. Treat it as advisory.

Reviewed the diff — no issues found. Swapping batch.column(0) for a shared first_column_data helper backed by columns().first() correctly removes the out-of-bounds panic on a zero-column batch, and both JSON→Arrow call sites are updated.

Minor test note: empty_schema_returns_err_not_panic asserts only is_err(), and a zero-field schema may already short-circuit earlier at the "no record batch in JSON" context before ever reaching first_column_data — so it validates the overall no-panic contract but doesn't guarantee the new helper is the branch actually exercised. If the intent is to pin the new helper specifically, constructing a batch that has a record but zero columns would target it directly. The single-field round-trip test is solid.


Generated by Claude Code

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