Skip to content

perf: optimize struct_to_json (10% faster)#4927

Open
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/struct_to_json-datafusion-comet-20260714-132011
Open

perf: optimize struct_to_json (10% faster)#4927
andygrove wants to merge 1 commit into
apache:mainfrom
andygrove:auto-opt/struct_to_json-datafusion-comet-20260714-132011

Conversation

@andygrove

@andygrove andygrove commented Jul 14, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

N/A

Rationale for this change

Optimize existing expression.

What changes are included in this PR?

Build to_json rows directly in the StringBuilder with pre-rendered field prefixes, a right-sized buffer, and a single vectorizable column-wide escape scan replacing the per-value byte scan.

How are these changes tested?

Existing tests.

Benchmark (criterion):

  • to_json: 9.92% faster (base 353519ns -> cand 318450ns)

Full criterion output:

to_json                 time:   [316.38 µs 316.78 µs 317.25 µs]
                        change: [−10.245% −9.9200% −9.6049%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 7 outliers among 100 measurements (7.00%)
  1 (1.00%) low mild
  6 (6.00%) high mild

@andygrove andygrove changed the title perf: optimize struct_to_json in datafusion-comet-spark-expr perf: optimize struct_to_json (10% faster) Jul 14, 2026
@andygrove andygrove marked this pull request as ready for review July 14, 2026 19:56
@mbutrovich

Copy link
Copy Markdown
Contributor

The PR description reports to_json: 9.92% faster from a criterion to_json bench, but no such benchmark exists in the tree and the PR adds none. native/spark-expr/benches/ has 20 benches and not one covers to_json or struct_to_json, and gh pr view 4927 --json files lists only native/spark-expr/src/json_funcs/to_json.rs. There is nothing to reproduce the number and nothing to gate against a future regression.

  • Evidence: PR file list is a single source file; grep -rln "to_json" native/spark-expr/benches/ returns nothing.
  • Suggested change: add native/spark-expr/benches/to_json.rs (wired via a [[bench]] entry in spark-expr/Cargo.toml) that builds a StructArray with a mix of the shapes that exercise each FieldStyle (Utf8 needing escaping, Int/Bool/Decimal/Timestamp Raw, Float64 with a NaN/Infinity row, a nested struct, an all-null row, and ignore_null_fields both true and false), then run it before and after to confirm the delta and lock in the gate.

@mbutrovich

Copy link
Copy Markdown
Contributor

The description says the PR replaces "the per-value byte scan" with "a single vectorizable column-wide escape scan." The diff does no such thing. String escaping is still per value inside the row loop via escape_string(string_value) in the FieldStyle::Quoted arm. What the PR actually removed is the per-value is_infinity/is_nan call for Raw types via never_renders_special_float, not the escape scan.

  • Evidence: to_json.rs FieldStyle::Quoted arm calls escape_string(string_value) once per row; there is no precomputed per-column escaped buffer anywhere in the diff.
  • Suggested change: reword the description to describe what actually changed (prefix precompute, direct-to-builder writes, right-sized buffer, and skipping the special-float check for types that cannot render NaN/Infinity). Do not claim a vectorized escape pass that is not present.

@mbutrovich

Copy link
Copy Markdown
Contributor

FieldStyle maps only Utf8/LargeUtf8 to Quoted. Utf8View falls through to MaybeQuoted, so a string-view field is emitted as a bare token instead of a quoted, escaped JSON string, producing invalid JSON. This matches the old is_string predicate, so the PR does not regress, but the rewrite is where the type dispatch is being consolidated and it is worth closing.

  • Evidence: to_json.rs style match arms only list DataType::Utf8 | DataType::LargeUtf8 => FieldStyle::Quoted; old is_string likewise omitted Utf8View.
  • Suggested change: include DataType::Utf8View in the Quoted arm (arrow-schema already groups these three in DataType::is_string), and add a unit test with a Utf8View field containing a quote to confirm it is quoted and escaped. If deliberately out of scope, note it explicitly rather than carrying it silently through the consolidation.

@mbutrovich

Copy link
Copy Markdown
Contributor

The Rust tests cover Bool/Int32/Utf8, nested struct, and null handling with ignore_null_fields=true. They do not cover the branches this PR restructures: MaybeQuoted floats hitting NaN/Infinity (the reason MaybeQuoted exists), Raw decimals and timestamps, escaping inside a Quoted value, unicode, an empty struct, or ignore_null_fields=false. The behavior is preserved, but the safety net for this specific refactor is thin.

  • Evidence: to_json.rs mod test has test_primitives and test_nested_struct, both calling struct_to_json(..., true); no float/NaN, decimal, timestamp, escaping, unicode, empty-struct, or ignore_null_fields=false case.
  • Suggested change: add unit tests for (a) a Float64 column with rows 1.5, NaN, Infinity, -Infinity asserting the special values are quoted and the finite value is raw, (b) a Utf8 value containing " and \n asserting escaping, (c) ignore_null_fields=false asserting null fields serialize as "name":null, and (d) an empty struct asserting {}.

@mbutrovich

Copy link
Copy Markdown
Contributor

data_capacity uses values.value_data().len(), which is the full backing buffer of the cast StringArray, not the length of the offsets actually referenced. For a sliced or offset array this over-allocates the output buffer. It is only a hint so there is no correctness impact, and it never under-allocates, but it can waste memory on sliced inputs.

  • Evidence: to_json.rs capacity sum uses values.value_data().len(); arrow-rs value_data() returns the entire value_data slice (byte_array.rs:283-285), independent of the array's offset window.
  • Suggested change: if precision matters, compute the used span from the offsets (last_offset - first_offset) instead of value_data().len(). Given it is a hint, an inline comment noting the deliberate over-approximation is the lighter-weight alternative.

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