Skip to content

owned_value_to_serde_json clones every string leaf instead of moving an already-owned value #4308

Description

@mattp5657

Description

core/connectors/sdk/src/convert.rs::owned_value_to_serde_json takes
&simd_json::OwnedValue and recursively allocates a fresh String
for every string leaf (s.to_string()) and every object key
(k.to_string()) it visits:

pub fn owned_value_to_serde_json(value: &simd_json::OwnedValue) -> serde_json::Value {
    match value {
        simd_json::OwnedValue::Static(s) => match s { /* ... */ },
        simd_json::OwnedValue::String(s) => serde_json::Value::String(s.to_string()),
        simd_json::OwnedValue::Array(arr) => {
            serde_json::Value::Array(arr.iter().map(owned_value_to_serde_json).collect())
        }
        simd_json::OwnedValue::Object(obj) => {
            let map: serde_json::Map<String, serde_json::Value> = obj
                .iter()
                .map(|(k, v)| (k.to_string(), owned_value_to_serde_json(v)))
                .collect();
            serde_json::Value::Object(map)
        }
    }
}

At most call sites the caller already owns the OwnedValue outright
and drops it immediately after converting. Converting between
OwnedValue and serde_json::Value means rebuilding the tree either
way, since they're unrelated types, but the leaf strings and object
keys don't need to ride along with that rebuild: they're already-owned
heap allocations one line away from being dropped, so .to_string()
copies their bytes into a new allocation just to throw the original
away right after.

Affected area / component

Connectors

Proposed solution

Add a consuming variant, additive (no breaking signature change):

pub fn owned_value_into_serde_json(value: simd_json::OwnedValue) -> serde_json::Value {
    match value {
        simd_json::OwnedValue::Static(s) => match s { /* unchanged, Copy types */ },
        simd_json::OwnedValue::String(s) => serde_json::Value::String(s.into()),
        simd_json::OwnedValue::Array(arr) => {
            serde_json::Value::Array(arr.into_iter().map(owned_value_into_serde_json).collect())
        }
        simd_json::OwnedValue::Object(obj) => {
            let map: serde_json::Map<String, serde_json::Value> = obj
                .into_iter()
                .map(|(k, v)| (k.into(), owned_value_into_serde_json(v)))
                .collect();
            serde_json::Value::Object(map)
        }
    }
}

Keep the existing borrow-based function for call sites that only have
a &OwnedValue. Swap opensearch_sink, http_sink, both
surrealdb_sink sites, and avro.rs to the consuming function
directly. Leave s3_sink and delta_sink for a follow-up, since they
need their callers restructured to own the payload first (see "Where
it occurs"). convert.rs already has an 11-case test module covering
every OwnedValue variant for the existing borrow-based function; the
new function's tests can mirror those directly rather than being
written from scratch. Should land as its own PR, not folded into an
unrelated sink's branch.

Validation

Check Result
Code Confirmed at 4/6 call sites: caller already owns the value and drops it right after (see "Where it occurs").
Object keys OwnedValue::Object = Box<HashMap<String, Value, ObjectHasher>>, plain String keys, no Cow/beef. .into() in the fix is a plain move.
Upstream No existing apache/iggy issue/PR found via GitHub's issue search API.

Benchmark: standalone crate (serde_json 1.0.151, simd-json 0.18.1, release + lto), converting a pre-built Vec<OwnedValue> of
1000 messages x 200 batches, one ~1 KB document each (25 x 24-byte
string fields); only the convert-and-drop loop is timed, cloning
happens outside it:

per message per 1000-msg batch
current (borrow) ~6.1-6.2 us ~6.1-6.2 ms
proposed (consume) ~3.3 us ~3.3 ms

Stable across 3 runs, dev machine (not a bench box, directional, not
a committed number). ~2.8 ms/batch is real but small next to the
network I/O (bulk request, upload, query) each sink does right after,
so it won't move end-to-end latency under normal load; matters more
under sustained high throughput or with the network stubbed out.

Contribution

  • I'm willing to submit a pull request to implement this feature

Good first issue

  • I think this could be a good first issue for a new contributor

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions