[SPARK-51975][SQL] Add variant_from_arrays and variant_from_entries - #57497
[SPARK-51975][SQL] Add variant_from_arrays and variant_from_entries#57497SreeramaYeshwanthGowd wants to merge 10 commits into
Conversation
| select cast(variant_from_arrays(cast(null as array<string>), array(1)) as string); | ||
| -- A null key is rejected. | ||
| select variant_from_arrays(array('a', cast(null as string)), array(1, 2)); | ||
| -- Duplicate keys are rejected. |
There was a problem hiding this comment.
Please note a minor e2e test coverage gap: the SQL golden input documents -- Duplicate keys are rejected for variant_from_arrays (select variant_from_arrays(array('a', 'a'), array(1, 2))) but has no analogous duplicate-key case for variant_from_entries; the entries cases stop at null-key / wrong-type / uncastable-value.
There was a problem hiding this comment.
@uros-b Thanks, Added a duplicate-key case for variant_from_entries and regenerated the golden output so it mirrors the variant_from_arrays case.
| // scalastyle:off line.size.limit | ||
| @ExpressionDescription( | ||
| usage = "_FUNC_(keys, values) - Creates a variant object from the given arrays of keys and values. The keys must be non-null strings and the two arrays must have the same length.", | ||
| examples = """ |
There was a problem hiding this comment.
Can we have arguments docs?
There was a problem hiding this comment.
@HyukjinKwon, added arguments docs for both functions. Thank you!
…rom_arrays/entries
…ases for variant_from_entries
b0ca2f7 to
59acb12
Compare
|
@cloud-fan Would you have a moment to review this when you get a chance? Thank you! |
cloud-fan
left a comment
There was a problem hiding this comment.
0 blocking, 1 non-blocking, 0 nits.
The feature is coherent and well covered; one small hot-path allocation cleanup is worth making before completion.
Suggestions (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:297: These helpers execute once per input row, so allocating Scala Range/foreach machinery adds row-cardinality overhead. Please cache
numElements()and use index-basedwhileloops in both helpers, matching the surrounding Catalyst evaluation style. -- see inline
Verification
I traced both public entry forms through FunctionRegistry, Catalyst type checking, interpreted and generated evaluation, and the shared VariantBuilder conversion. The new paths preserve the established map/to_variant_object behavior for valid inputs, nulls, unsupported types, and errors, with the intentional stricter duplicate-key behavior required by Variant objects.
| val builder = new VariantBuilder(false) | ||
| val start = builder.getWritePos | ||
| val fields = new java.util.ArrayList[VariantBuilder.FieldEntry](keys.numElements()) | ||
| for (i <- 0 until keys.numElements()) { |
There was a problem hiding this comment.
Please cache numElements() and use index-based while loops here and in the matching entries loop at VariantExpressionEvalUtils.scala:322. These helpers execute once per input row, so 0 until ... adds avoidable Range/foreach allocation in a Catalyst hot path.
…omArrays/variantFromEntries
|
@cloud-fan Thanks, cached numElements() and switched both loops (variantFromArrays and the matching variantFromEntries loop at line 322) to index-based while loops. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 1 new to this AI review. (0 newly introduced, 1 late catch, 0 previously raised.)
0 blocking, 1 non-blocking, 0 nits.
The prior hot-loop allocation issue is fixed; one additional single-pass efficiency cleanup remains.
Suggestions (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/variantExpressions.scala:323: When the entries element type permits nulls, every non-null row scans the full array once here and then scans it again in variantFromEntries. Fold the null-entry check into the construction loop so this row-cardinality hot path remains single-pass. -- see inline
Verification
I traced both public entry forms through FunctionRegistry, Catalyst type checking and nullability, interpreted and generated evaluation, and VariantBuilder object finalization. The current head preserves the documented null, duplicate-key, length, and unsupported-type behavior, and the previous Range/foreach allocation concern is resolved. For nullable entry arrays, however, nullSafeEval still pre-scans the full array before variantFromEntries performs its construction scan.
| val entries = input.asInstanceOf[ArrayData] | ||
| if (nullEntries) { | ||
| var i = 0 | ||
| while (i < entries.numElements()) { |
There was a problem hiding this comment.
Fold this null-entry check into variantFromEntries and propagate a null helper result into the expression's null flag. With containsNull=true, this loop walks every non-null row's full array, then the helper immediately walks it again to build the object, leaving this hot path with two cardinality-scaled traversals.
|
@cloud-fan Thanks, folded the null-entry check into variantFromEntries's existing loop. It now returns null on a null entry, and both nullSafeEval and codegen just propagate that result instead of pre-scanning, so the whole path is single-pass again. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 1 new to this AI review. (1 newly introduced, 0 late catches, 0 previously raised.)
1 blocking, 0 non-blocking, 0 nits.
The public surface and implementation are coherent, but the single-pass nullable-entry change alters error ordering for values before a later null entry.
Correctness (1)
- sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/variant/VariantExpressionEvalUtils.scala:328: Preserve null-entry dominance before converting preceding values; the new single-pass loop can throw before reaching a later null entry. -- see inline
Verification
I traced both public entry forms through registration, type checking, interpreted/code-generated evaluation, recursive value conversion, and VariantBuilder object finalization. The new single-pass entries loop converts each value before inspecting later entries, while the established map_from_entries analogue pre-scans nullable entries before processing values; this makes a later null no longer dominate an earlier conversion failure.
| val fields = new java.util.ArrayList[VariantBuilder.FieldEntry](numElements) | ||
| var i = 0 | ||
| while (i < numElements) { | ||
| if (entries.isNullAt(i)) { |
There was a problem hiding this comment.
Please preserve the documented null-entry dominance before converting any values. In this single-pass loop, an earlier nested value can fail during buildVariant (for example, a struct with duplicate field names) before a later null entry is reached, so the call throws instead of returning null as map_from_entries does; retaining the pre-scan and adding a mixed failing-value/null test would keep the contract intact.
…ariantFromEntries Restores the null-only pre-scan inside variantFromEntries so a null entry always makes the result null even when an earlier entry's value would fail to convert, matching map_from_entries. Also fixes VariantFromEntries codegen to only reassign ev.isNull when the expression is nullable, since nullSafeCodeGen does not declare that local otherwise.
|
@cloud-fan Thanks, good catch. Restored the null-only pre-scan in variantFromEntries so a null entry always dominates an earlier conversion failure, matching map_from_entries, and added a test for that case. |
cloud-fan
left a comment
There was a problem hiding this comment.
1 addressed, 0 remaining, 0 new to this AI review.
0 blocking, 0 non-blocking, 0 nits.
The previously reported null-entry error-ordering regression is fixed, and the current public API, implementation, generated-code path, and test portfolio are coherent.
Verification
I traced both public entry forms from FunctionRegistry and language wrappers through Catalyst type checking, interpreted evaluation, generated evaluation, recursive Variant conversion, and object finalization. I also compared nullable-entry behavior with map_from_entries: both pre-scan for null entries before processing values, and the added focused test covers a conversion failure before a later null entry. The selected prose, contract, and local-efficiency scans reported no current findings.
What changes were proposed in this pull request?
Add two built in scalar SQL functions that construct a Variant object directly from collections:
variant_from_arrays(keys, values)builds a Variant object from an array of string keys and an array of values.variant_from_entries(entries)builds a Variant object from an array of key/value struct entries (a two field struct per entry).API surface added:
variant_from_arrays,variant_from_entriesfunctions.variant_from_arrays(keys, values),functions.variant_from_entries(entries)pyspark.sql.functions.variant_from_arrays/variant_from_entriesImplementation notes:
variantExpressions.scalanext toto_variant_object, delegating (eval and codegen) to two newVariantExpressionEvalUtilshelpers that build the object in a single pass withVariantBuilder(addKey/finishWritingObject) and reuse the existingbuildVariantfor each value. This avoids materializing an intermediateMapType, which is the point of the ticket.to_variant_objectandmap_from_*precedents:to_variant_objectdoes for non-string-keyed maps, it is not implicitly cast); a null key raisesNULL_MAP_KEY.VARIANT_DUPLICATE_KEY, matchingto_variant_object(Variant objects cannot hold duplicate keys). This is stricter thanmap_from_*under the non defaultspark.sql.mapKeyDedupPolicy=LAST_WIN.NULL, and forvariant_from_entriesa null entry yieldsNULL(asmap_from_entriesdoes).variant_from_arrays, a keys/values length mismatch reuses the same errormap_from_arraysraises.Why are the changes needed?
Today, creating a Variant object from arrays or entries requires a two step expression:
to_variant_object(map_from_arrays(...))orto_variant_object(map_from_entries(...)). That materializes an intermediateMapTypeonly to immediately convert it.variant_from_arrays/variant_from_entriesbuild the Variant object directly, reducing allocation and CPU, which matters for large maps and high volume workloads. They also mirror Spark's ownmap_from_arrays/map_from_entriesnaming, so the Variant surface stays consistent.Does this PR introduce any user-facing change?
Yes. It adds two new built in SQL functions and their Scala and PySpark DataFrame API entries. No existing behavior changes.
How was this patch tested?
VariantExpressionSuitecovering object construction, key sorting, empty input, null values, nested values, null array / null entry inputs returning null, and the null key / duplicate key / length mismatch errors.VariantEndToEndSuiteexercising the DataFrame API under bothCODEGEN_ONLYandNO_CODEGENover a non foldable relation.variant/variant-from-arrays-entries.sql, covering each JSON type, null handling, and the null key / duplicate key / length mismatch / non-string key / unsupported value type errors.test_functions.pytest, and regeneratedsql-expression-schema.md.Was this patch authored or co-authored using generative AI tooling? No