perf(arrow codec): drop serde_json::Value round-trip in Arrow encoding#25773
perf(arrow codec): drop serde_json::Value round-trip in Arrow encoding#25773benjamin-awd wants to merge 2 commits into
Conversation
Arrow's JSON decoder accepts any `Serialize` type, so there is no need to first materialize each event into a `serde_json::Value` tree. Serialize `&LogEvent`s straight into the decoder, removing one tree allocation and one `Serialize` walk per event on the encode hot path. `build_record_batch` and `find_null_non_nullable_fields` now take `&[&LogEvent]`. `vector_log_events_to_json_values` is retained only for the Parquet auto-infer path (Arrow's JSON schema inference still needs it) and is gated behind the `parquet` feature. Behavior is preserved: all type coercion still runs through the unchanged Arrow JSON decoder via the identical `Serialize` path. A Criterion bench on a ~10 MB batch of wide/nested events shows encode time drop ~30% (record-batch build 27.8ms -> 19.3ms, end-to-end IPC 29.3ms -> 20.4ms).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1dacc9a1ed
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let json_values = match vector_log_events_to_json_values(&events) { | ||
| Ok(values) => values, |
There was a problem hiding this comment.
In schema_mode = auto_infer, this still infers from serde_json::Values while the batch is now encoded from the original LogEvents. serde_json::to_value converts non-finite floats such as inf/-inf to null, so a batch where a field contains only those values is inferred as an Arrow Null column; the subsequent build_record_batch(..., &logs) then tries to decode the original float into that Null column and the parquet encode errors instead of producing the new pass-through behavior.
Useful? React with 👍 / 👎.
benjamin-awd
force-pushed
the
perf/arrow-drop-json-roundtrip
branch
from
f3c9e85 to
6803d82
Compare
In AutoInfer mode the schema is inferred from serde_json::Values, which coerce non-finite floats (inf/-inf) to null and therefore infer a Null column. After dropping the JSON round-trip on the encode path, the record batch was built from the original log events instead, feeding those raw floats into the inferred Null column, which the decoder rejects. Encode AutoInfer batches from the same JSON values used for inference so the data and schema always agree. Strict/Relaxed keep the direct log-event path.
benjamin-awd
force-pushed
the
perf/arrow-drop-json-roundtrip
branch
from
6803d82 to
4fc638e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4fc638ed43
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| self.schema = Arc::new(ParquetSchemaGenerator::try_normalize_schema( | ||
| &events, schema, | ||
| )); | ||
| decode_rows_to_record_batch(Arc::clone(&self.schema), &json_values) |
There was a problem hiding this comment.
For schema_mode = auto_infer, the batch is still encoded from serde_json::Values, so the new direct encoder never sees non-finite floats. If a Parquet batch contains inf/-inf, serde_json::to_value has already converted those values to null; with a finite value in the same batch the column can still infer as Float64 but the non-finite rows are written as null, and with only non-finite values the field becomes Null. Fresh evidence in this revision is the explicit decode_rows_to_record_batch(..., &json_values) call here after schema inference, so auto-infer still differs from relaxed/strict and from the pass-through behavior described for this change.
Useful? React with 👍 / 👎.
1 participant
Summary
The Arrow codec used to build each record batch in two passes:
The first pass is unnecessary. Arrow's decoder accepts anything that implements
Serialize, andLogEventalready does, so events can go straight into the decoder:Vector configuration
No configuration changes.
How did you test this PR?
Ran unit/integration tests for Arrow + Parquet
Change Type
Is this a breaking change?
Technically a breaking change, since non-finite floats (
inf/-inf) are now passed through to the destination, whereas previously they were encoded as null.This behavior is arguably more correct and unlikely to affect users.
Does this PR include user facing changes?
no-changeloglabel to this PR.References
Notes
@vectordotdev/vectorto reach out to us regarding this PR.pre-pushhook, please see this template.make fmtmake check-clippy(if there are failures it's possible some of them can be fixed withmake clippy-fix)make testgit merge origin masterandgit push.Cargo.lock), pleaserun
make build-licensesto regenerate the license inventory and commit the changes (if any). More details on the dd-rust-license-tool.