Skip to content

feat: unify graph JSON output on one Arrow writer and offer Arrow IPC as a query result format - #627

Merged
azimafroozeh merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:json-results-via-arrow-json
Sep 5, 2026
Merged

feat: unify graph JSON output on one Arrow writer and offer Arrow IPC as a query result format#627
azimafroozeh merged 4 commits into
ModernRelay:mainfrom
azimafroozeh:json-results-via-arrow-json

Conversation

@azimafroozeh

Copy link
Copy Markdown
Contributor

What & why

Closes #618. This PR implements RFC 0051 (#622): every JSON body OmniGraph writes for graph data is the arrow-json rendering of the result batches with the crate's default options, both hand-written cell encoders are deleted, and the query routes serve Arrow IPC on request.

  • Phase 1, query results: QueryResult::to_json_bytes() is arrow_json::WriterBuilder with default options over the result batches; to_rust_json() and deserialize() parse those bytes. ReadOutput.rows and LegacyReadOutput.rows carry the rendered text as Box<RawValue>: the three query handlers pass it through, the CLI parses it for table, csv, and kv, and the OpenAPI schema for rows is unchanged. The compiler's per-type encoder, JsonIntegerMode, and every to_sdk_json are removed.
  • Phase 2, export, entity fetch, change images: QueryResult::to_json_lines() (one object per line) renders the export data object, entity_at, and the before/after images of POST /changes; the engine's encoder is deleted. OmniGraph keeps four pieces of its own: the base64: blob substitution before the writer, the export line envelope, the date-range check below, and the re-insertion of explicit nulls into change images (there an absent key means the property was outside that commit's schema). Export renders 256 rows per writer call; the data key order becomes id first then the catalog's column order (it was alphabetical).
  • Phase 3, Arrow IPC on the query routes, is in flight on this branch: omnigraph-api-types carries ARROW_STREAM_MEDIA_TYPE and the four envelope header constants, the server the 406 constructor, the CLI the arrow format variant. The Accept negotiation in POST /query and stored-query invocation, the CLI --format arrow byte lane, their tests, and the OpenAPI regeneration land before this PR opens; POST /read ignores Accept and a stored mutation answers JSON whatever Accept says.
  • Spellings a consumer sees change as the phase 1 and phase 2 rows of the RFC's behavior table: a null cell's key is omitted, DateTime drops the Z and a zero fractional part, F32 and Vector(N) print at 32-bit width (the bug: F32 values come back with widened digits #618 defect), exponent form is 1.0e20 from 1e10 up and below 1e-5, a non-finite computed float is null; export and entity fetch spell dates as strings instead of raw counts (the loader already read both forms). The release note and docs/user/queries/index.md §JSON result spelling carry the full list.
  • One check stays OmniGraph's: a Date or DateTime count arrow-json cannot format (it writes an ERROR: string and returns Ok) is refused before the writer, on the routes with status 500 naming the column, rows[i] across the whole result, and the count, on export, entity fetch, and change images prefixed with the entity id; the loader refuses the same counts on import. All go through the one predicate pair date32_renderable/date64_renderable.
  • The golden test every_catalog_type_renders_to_the_documented_spelling pins checked-in bytes for every Arrow type the catalog can produce, so an arrow-json upgrade that changes a spelling fails a test instead of changing the wire.
  • RFC 0051 is amended in this PR to its end state: implementation: complete, the change feed named as the third consumer, the export key order, the T25 refusal that landed in fix(compiler): refuse two projections that execute to one result column name #621, and one decision-log entry.

Backing issue / RFC

Checklist

  • Change is focused (one writer replaces both cell encoders; the Arrow IPC route exposes the same batches the writer renders)
  • Tests added/updated for behavior changes (golden bytes per catalog type including non-finite floats and both exponent thresholds at both widths; row_index_counts_across_batches_and_only_rendered_slots_are_checked; loader refusal at i32/i64 MIN and MAX; signed-year DateTime strings re-import; export_jsonl_round_trips_branch_snapshot pins one line's bytes and key order; export_jsonl_with_blob_type asserts base64:, the external URI, and the omitted null key on entity_at; CLI json_format_prints_the_envelope_pretty_and_the_rows_compact; server query_rows_omit_null_cells; the .gqt case; every in-tree caller of to_rust_json takes the Result)
  • Public docs updated if user-facing surface changed (docs/releases/v0.11.0.md phase 1 and phase 2 bullets, docs/user/queries/index.md §JSON result spelling, docs/user/cli/reference.md, skills/omnigraph/SKILL.md, RFC 0045's comparison sentence, RFC 0051 amendment, openapi.json descriptions)
  • Reviewed against docs/dev/invariants.md — no Hard Invariant weakened, no deny-list item hit (the "swallowed errors" deny-list entry is the shape removed: the old encoders wrote "" or a silent null for a display or downcast failure, the writer path returns a typed error; the IPC route serializes batches after the same policy and projection as JSON)

Local verification

  • cargo test --workspace --locked --no-fail-fast: 102 binaries, 2949 passed, 24 ignored; one failure, blob::tests::external_blob_file_policy_rejects_special_files, sandbox-only (UnixListener::bind EPERM), file untouched
  • cargo test -p omnigraph-gqt: 11 cases pass; issue_618_json_f32_datetime_null_spelling.gqt fails on main @ a09176d7 (the bug: F32 values come back with widened digits #618 symptom)
  • cargo test -p omnigraph-server openapi after regenerating openapi.json with OMNIGRAPH_UPDATE_OPENAPI=1: green, including error_code_schema_has_expected_variants; the vocabulary guard's openapi_inventory_matches test: green
  • cargo clippy --workspace --all-targets: one dead_code warning, ApiError::not_acceptable, until the phase 3 handlers wire it
  • cargo fmt --all --check: clean
  • cargo check --workspace --all-targets --locked: clean (arrow-json = "58" adds a workspace edge only; the crate was already in the lock through arrow)
  • Phase 3 handler, CLI lane, and their server and CLI tests: not yet written at this draft; the suite lines above are re-run and replaced before the PR opens
  • scripts/check-docs.py on the amended RFC: not yet run; the RFC edit lands after the rebase onto main
  • DST suite and s3_storage tests: not run (bucket-gated and long-running); both compile under --all-targets
  • vocabulary guard, rust-string surface: not run against this base; it reports 160 pre-existing findings at the merge base, none in a file this PR touches

Notes for reviewers

  • Writer defaults everywhere: no with_explicit_nulls, no date format strings. Each default is one of the listed spelling changes; a per-type override would be OmniGraph deciding a spelling again, which is what the RFC removes. The one exception is the change feed, where logical_row_image re-inserts null after the writer because the CLI diff's <absent> marker means "outside that commit's schema".
  • Render errors on the routes map to ApiError::internal (500), not the OmniError::Compiler 400 path: a value the writer cannot render is a server-side failure, not a bad request. On export the stream ends at a 256-row render-window boundary before the failing row, every emitted line complete, and the call returns the typed error naming the entity; the file carries no in-band truncation signal.
  • Legacy stores: a Date or DateTime count loaded before this release that the writer cannot format fails every read, export, entity fetch, and change image touching that column, each naming the entity or rows[i] and the count, until that row is updated. Accepted: the renderable range sits about 30x beyond any calendar-expressible date, no literal, parameter, or arithmetic route can produce such a count, and the named row plus update is the repair. The Arrow IPC route serializes the stored count without the range check and is the read-side escape hatch.
  • The refusal's honest routes: ISO strings, day and millisecond counts, date()/datetime() literals, now(), and Date/DateTime parameters all pass. What it does not stop, by design: a filter on an unprojected date column runs on the raw count, which is also the repair route; the IPC route carries the count unformatted.
  • RawValue::from_string is one validation pass over the rendered bytes, so rows are validated once and never parsed into a value tree; the RFC's "carried as bytes" is one pass, not zero.
  • The loader refusal is pinned at the parser unit and by the signed-year re-import test; a load_jsonl-level refusal of an out-of-range count is not pinned in the diff.
  • The export line envelope is concatenated by hand around the writer's row bytes; a typed line struct would need a &RawValue from those bytes, one more validation pass per line, and export_jsonl_round_trips_branch_snapshot pins the grammar at the byte level.
  • ErrorCode gains no NotAcceptable variant: the enum is a closed wire contract, so 406 carries BadRequest, the 415 precedent.
  • Engine footprint: entity_at is now a with_row_id bounded scan rendered by the same writer, so the read-only .dataset() count for db/omnigraph/export.rs in the forbidden_apis chokepoint registry goes from 1 to 2 (the second call is the entity blob read).
  • The new error strings name column and rows; no vocabulary-inventory row is added, since the rust-string guard cannot run at this merge base and the terms name a result column, not storage.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@azimafroozeh azimafroozeh changed the title feat: unify graph JSON output on one Arrow writer and offer Arrow IPC… feat: unify graph JSON output on one Arrow writer and offer Arrow IPC as a query result format Sep 5, 2026
@azimafroozeh
azimafroozeh force-pushed the json-results-via-arrow-json branch from 4399c80 to d75232b Compare September 5, 2026 10:03
@azimafroozeh
azimafroozeh merged commit 9570c7a into ModernRelay:main Sep 5, 2026
23 checks passed
@azimafroozeh
azimafroozeh deleted the json-results-via-arrow-json branch September 5, 2026 10:49
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.

bug: F32 values come back with widened digits

1 participant