[SPARK-57804][SQL] Add variant_set expression - #57125
Conversation
variant_set expressionvariant_set expression
variant_set expressionvariant_set expression
shrirangmhalgi
left a comment
There was a problem hiding this comment.
Design question (non-blocking)
When createIfMissing is false, a type mismatch (e.g., object-key segment hitting a scalar) throws VARIANT_PATH_TYPE_MISMATCH. Should it instead leave the variant unchanged (same as missing-key behavior)?
PostgreSQL's jsonb_set(target, path, new_value, create_missing := false) returns target unchanged when a path is unreachable -- the docs state: "If these conditions are not met the target is returned unchanged." This covers both missing intermediates AND type mismatches.
The current behavior is internally consistent (createIfMissing=true also throws on type mismatch), but users migrating from PostgreSQL may expect silent passthrough when create_if_missing is false.
Not blocking - just flagging since it's a user-facing semantic difference from the ecosystem precedent.
Good point, however trying to override value of a different type like this seems either like mistake or genuine intention that should not be silent. Missing paths ("field doesn't exist yet") vs wrong-type paths ("your assumption about the shape is wrong") are semantically distinct signals. That said, we will support try version that will catch type mismatch, so we will have parity. Also, PostgreSQL is not really consistent with itself as it does error if intermediate path type is array where object is expected. |
harshmotw-db
left a comment
There was a problem hiding this comment.
Implementation LGTM. I'm wondering if rejecting $ is the right idea from an API PoV - maybe a user could want to replace the whole variant in some rows but only replace nested fields in other rows since we allow dynamic paths
|
@shrirangmhalgi I also prefer the current approach if migration-friendliness is not a blocker. Silently allowing invalid paths feels wrong |
Good point, the reason I decided to reject it is because this way cc @srielau |
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 0 non-blocking, 0 nits.
Clean, well-tested addition that faithfully follows the established variant_insert pattern. LGTM.
Verification
Traced the appendWithSetImpl variant-value rebuild against every input-shape cell: object/array container, target present vs. absent, leaf vs. intermediate segment, array index in-range / at-end / past-end, and both create_if_missing values. Every cell is either shown correct or gated (the create_if_missing=false no-op cases are guarded by !found && createIfMissing and if (createIfMissing), with the copied fields/elements reproducing the input), and each is backed by a test; kind-mismatch throws VARIANT_PATH_TYPE_MISMATCH and size overflow raises VARIANT_SIZE_LIMIT. Codegen and interpreted paths are exercised under both CODEGEN_ONLY and NO_CODEGEN, and nullability correctly uses the QuaternaryExpression default (no try-mode, so no nullable override needed unlike variant_insert).
No new findings. The three open design questions (reject root $; type-mismatch behavior when create_if_missing=false; Parsed*Path companion-class duplication) are already raised by reviewers and answered by the author, so I'm not duplicating them.
### What changes were proposed in this pull request? Adds the SQL function `variant_set(v, path, val[, create_if_missing])`, which sets (inserts or replaces) a value in a Variant value at a single JSONPath location. Details: - Object path (e.g. `$.a`): replaces the field if it exists; creates it when `create_if_missing` is true (the default); - Array path (e.g. `$[N]`): replaces the element at index `N`; when `N` is at or past the end and `create_if_missing` is true, the array is padded with variant nulls up to `N`; - Missing intermediate keys/indices along the path are created when create_if_missing` is true; - When `create_if_missing` is false, a missing leaf, a missing intermediate, or an out-of-range array index leaves `v` unchanged; - Any NULL argument returns NULL; - The value may be any expression castable to variant (primitives, arrays, or another variant; structs and maps are rejected); - `VARIANT_PATH_TYPE_MISMATCH` is raised when a path segment is applied to a value of an incompatible type; the root path `$` is rejected with `INVALID_VARIANT_PATH`, and results exceeding the size limit raise `VARIANT_SIZE_LIMIT`. ### Why are the changes needed? Without `variant_set`, updating a variant means converting it to another datatype (e.g. map), mutating, and converting back. ### Does this PR introduce _any_ user-facing change? Yes, a new SQL function (and Scala/Python `functions` API). ### How was this patch tested? Unit tests. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code with Claude Opus 4.8 Closes #57125 from bojana-db/variant-set. Authored-by: bojana-db <bojana.zecevic@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit f22a401) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
What changes were proposed in this pull request?
Adds the SQL function
variant_set(v, path, val[, create_if_missing]), which sets (inserts or replaces) a value in a Variant value at a single JSONPath location.Details:
$.a): replaces the field if it exists; creates it whencreate_if_missingis true (the default);$[N]): replaces the element at indexN; whenNis at or past the end andcreate_if_missingis true, the array is padded with variant nulls up toN;create_if_missingis false, a missing leaf, a missing intermediate, or an out-of-range array index leavesvunchanged;VARIANT_PATH_TYPE_MISMATCHis raised when a path segment is applied to a value of an incompatible type; the root path$is rejected withINVALID_VARIANT_PATH, and results exceeding the size limit raiseVARIANT_SIZE_LIMIT.Why are the changes needed?
Without
variant_set, updating a variant means converting it to another datatype (e.g. map), mutating, and converting back.Does this PR introduce any user-facing change?
Yes, a new SQL function (and Scala/Python
functionsAPI).How was this patch tested?
Unit tests.
Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code with Claude Opus 4.8