Skip to content

[SPARK-54698][SQL] Support hashing for all data types for ArraySetLike operations - #53468

Open
Kimahriman wants to merge 11 commits into
apache:masterfrom
Kimahriman:more-proper-equals
Open

[SPARK-54698][SQL] Support hashing for all data types for ArraySetLike operations#53468
Kimahriman wants to merge 11 commits into
apache:masterfrom
Kimahriman:more-proper-equals

Conversation

@Kimahriman

@KimahrimanKimahriman commented Dec 13, 2025

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Update all array set-like expressions to use hashing for all data types. Currently complex types and certain string collations fallback to a nested comparison loop that is O(n^2) for the size of the array, which can get really expensive for larger arrays. Instead, this borrows the idea from InternalRowComparableWrapper and creates a GenericComparableWrapper that works for any data type, using Murmur3HashFunction for the hash code and InterpretedOrdering for equals.

Additionally, since there is just one code path for these expressions now, I moved the eval logic directly into the nullSafeEval function instead of keeping the transient function call.

Why are the changes needed?

To improve performance of array set-like operations for complex types.

Local benchmark results:

Before

[info] Running benchmark: Array Distinct
[info] Running case: array_distinct
[info] Stopped after 2 iterations, 9548 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Distinct: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_distinct 4638 4774 192 0.0 4638443.2 1.0X
[info] Running benchmark: Array Union
[info] Running case: array_union
[info] Stopped after 2 iterations, 9092 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Union: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_union 4310 4546 334 0.0 4309904.1 1.0X
[info] Running benchmark: Array Except
[info] Running case: array_except
[info] Stopped after 2 iterations, 4260 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Except: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_except 2043 2130 123 0.0 2042714.3 1.0X
[info] Running benchmark: Array Intersect
[info] Running case: array_intersect
[info] Stopped after 2 iterations, 4847 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Intersect: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_intersect 2324 2424 142 0.0 2323510.6 1.0X

After

[info] Running benchmark: Array Distinct
[info] Running case: array_distinct
[info] Stopped after 2 iterations, 2108 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Distinct: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_distinct 947 1054 152 0.0 946800.6 1.0X
[info] Running benchmark: Array Union
[info] Running case: array_union
[info] Stopped after 2 iterations, 2772 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Union: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_union 929 1386 646 0.0 929444.2 1.0X
[info] Running benchmark: Array Except
[info] Running case: array_except
[info] Stopped after 6 iterations, 2194 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Except: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_except 331 366 49 0.0 330820.6 1.0X
[info] Running benchmark: Array Intersect
[info] Running case: array_intersect
[info] Stopped after 7 iterations, 2013 ms
[info] OpenJDK 64-Bit Server VM 17.0.17+10-LTS on Linux 4.18.0-553.89.1.el8_10.x86_64
[info] AMD EPYC Processor (with IBPB)
[info] Array Intersect: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_intersect 261 288 16 0.0 260571.4 1.0X

Does this PR introduce any user-facing change?

Set-like operations for complex types now enforce some limitations, such as the max array size, that were not enforced earlier, though it may have been unreachable in practice previously. Otherwise just performance improvement.

How was this patch tested?

New UTs for complex data.

Was this patch authored or co-authored using generative AI tooling?

Tests and benchmark created with GPT-5.4

@KimahrimanKimahriman changed the title [SPARK-54698] Support hashing for all data types for array set like operations[SPARK-54698][SQL] Support hashing for all data types for array set like operationsDec 13, 2025

@qlongqlong left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I am unsure if performance enhancement can be validated in CI.

@asugranyes

Copy link
Copy Markdown
Contributor

Hi, I have a dependent PR (#53695) that adds -0.0 normalization to array operations. Is there anything blocking this PR from being merged?

@LuciferYang

Copy link
Copy Markdown
Contributor

Thanks for the pr! The approach of using GenericComparableWrapper to unify the two code paths is clean and effective.

One suggestion: since this pr changes the underlying lookup mechanism for complex types (from O(n^2) linear comparison to hash-based), it would be great to add some test coverage for the newly affected paths.
Specifically, the following scenarios maybe not covered by existing tests:

  1. StructType elements — None of the four operations have tests with struct-typed array elements.
  2. Nested complex types — e.g., ArrayType(StructType(...)) as the element type.
  3. Nested NaN — e.g., array_distinct([struct(NaN), struct(NaN)]). Current NaN tests only cover top-level Float/Double NaN.
  4. Nested -0.0 vs 0.0 — e.g., array_distinct([struct(-0.0), struct(0.0)]). The existing -0.0 vs 0.0 tests are top-level only and only cover array_union/array_distinct (in array.sql), not
    array_intersect/array_except.

Also, it would be helpful to add a benchmark comparing the before/after performance for complex types (e.g., arrays of structs or binary elements at various sizes), so reviewers can see the concrete improvement
from the O(n^2) to O(n) change.

@LuciferYang

Copy link
Copy Markdown
Contributor

I created a simple benchmark to test:

objectArraySetLikeBenchmarkextendsSqlBasedBenchmark {
privatevalN=1000LprivatevalarrayElements=100000overridedefrunBenchmarkSuite(mainArgs: Array[String]):Unit= {
valbenchmark=newBenchmark(s"Array Set Like", N, output = output)
valarr= (1 to arrayElements).map(x =>Array(x, x)).toArray
benchmark.addCase("array_union", 10) { _ =>
spark.range(N)
.select(array_union(lit(arr), lit(arr)).alias("arr"))
.write
.format("noop")
.mode("append")
.save()
}
benchmark.run()
}
}

Before:

[info] Array Set Like: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_union 49269 52834 7220 0.0 49268846.2 1.0X

After:

[info] Array Set Like: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] array_union 1779 2609 562 0.0 1779167.5 1.0X

My personal suggestion is to include the relevant benchmarks in this pr

@Kimahriman

Copy link
Copy Markdown
ContributorAuthor

Added more tests for those cases

My personal suggestion is to include the relevant benchmarks in this pr

I wasn't sure what the threshold was for including a new benchmark in the repo, or what the right way to generate the results files were

@LuciferYang

Copy link
Copy Markdown
Contributor

Added more tests for those cases

My personal suggestion is to include the relevant benchmarks in this pr

I wasn't sure what the threshold was for including a new benchmark in the repo, or what the right way to generate the results files were

We can use the Run benchmarks action in our personal repository to generate benchmark test results. You can also paste the benchmark testing methods and results into the pr description.

image

Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz
Array Intersect: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
array_intersect 95 106 13 0.0 94730.0 1.0X

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add the benchmark results before this optimization and modification to the PR description?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added local before and after to the PR description

@asugranyes

Copy link
Copy Markdown
Contributor

Nice generalization to all types via the comparable wrapper.

It looks like ArraysOverlap is left out of this.

It splits on the same TypeUtils.typeWithProperEquals(elementType) predicate, but its fastEval uses a plain java.util.HashSet[Any] (works only for proper-equals types per its docstring), so complex types fall back to bruteForceEval — the O(n²) nested loop with ordering.equiv.

/** * A slower evaluation which performs a nested loop and supports all the data types.*/privatedefbruteForceEval(arr1: ArrayData, arr2: ArrayData):Any= {
varhasNull=falseif (arr1.numElements() >0&& arr2.numElements() >0) {
arr1.foreach(elementType, (_, v1) =>if (v1 ==null) {
hasNull =true
} else {
arr2.foreach(elementType, (_, v2) =>if (v2 ==null) {
hasNull =true
} elseif (ordering.equiv(v1, v2)) {
returntrue
}
)
})
}
if (hasNull) {
null
} else {
false
}

With the comparable wrapper, fastEval could handle all types, making the typeWithProperEquals split and bruteForceEval unnecessary — O(n) for complex types instead of O(n²). Worth a follow-up?

@Kimahriman

Copy link
Copy Markdown
ContributorAuthor

Nice generalization to all types via the comparable wrapper.

It looks like ArraysOverlap is left out of this.

It splits on the same TypeUtils.typeWithProperEquals(elementType) predicate, but its fastEval uses a plain java.util.HashSet[Any] (works only for proper-equals types per its docstring), so complex types fall back to bruteForceEval — the O(n²) nested loop with ordering.equiv.

/** * A slower evaluation which performs a nested loop and supports all the data types.*/privatedefbruteForceEval(arr1: ArrayData, arr2: ArrayData):Any= {
varhasNull=falseif (arr1.numElements() >0&& arr2.numElements() >0) {
arr1.foreach(elementType, (_, v1) =>if (v1 ==null) {
hasNull =true
} else {
arr2.foreach(elementType, (_, v2) =>if (v2 ==null) {
hasNull =true
} elseif (ordering.equiv(v1, v2)) {
returntrue
}
)
})
}
if (hasNull) {
null
} else {
false
}

With the comparable wrapper, fastEval could handle all types, making the typeWithProperEquals split and bruteForceEval unnecessary — O(n) for complex types instead of O(n²). Worth a follow-up?

Probably would be good for a follow on, this specifically is targeting the things extending ArraySetLike, not sure why that method doesn't extend that

@Kimahriman
Kimahrimanforce-pushed the more-proper-equals branch 2 times, most recently from f4c3d01 to 6356fbaCompareJune 5, 2026 19:11
…etLikeBenchmark (JDK 17, Scala 2.13, split 1 of 1)
…etLikeBenchmark (JDK 25, Scala 2.13, split 1 of 1)
select trim_array(array(1, 2, 3), CAST(null AS INT));

-- SPARK-54698: Confirm 0.0, -0.0, and NaN are handled appropriately for complex types.
select array_union(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new SQL cases cannot exercise the wrapper's -0.0/NaN normalization: NormalizeFloatingNumbers runs in the FinishAnalysis batch, before ConstantFolding can fold anything, and it wraps both literals and column data, so the operator never sees raw bits via SQL execution; the 0.0 in the golden output (instead of -0.0) is the tell. To test that defensive logic, use Scala-side values in CollectionExpressionsSuite, e.g. a struct containing -0.0 and Double.longBitsToDouble(0x7ff0000000000001L); checkEvaluation bypasses the optimizer, so the bits reach the operator as-is.

Two more gaps: collated strings nested inside structs/arrays have no coverage anywhere, and RTRIM collations ('a' vs 'a ' under UTF8_BINARY_RTRIM) are untested for these functions. Direct collated-string elements are already covered by collations-basic.sql, worth confirming those still pass.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in commit c4fd1b9. I added Scala-side CollectionExpressionsSuite coverage for nested structs containing -0.0 and non-canonical NaN payloads across array_distinct, array_union, array_intersect, and array_except. I also added CollationExpressionSuite coverage for collated strings nested in structs and arrays, plus UTF8_BINARY_RTRIM cases such as 'a' versus 'a ' across the affected functions. The focused catalyst suites pass (74/74), and SQLQueryTestSuite passes both collations-basic.sql and collations-basic.sql_analyzer_test.



val arrayDistinctBenchmark = new Benchmark("Array Distinct", N, output = output)
arrayDistinctBenchmark.addCase("array_distinct") { _ =>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each benchmark group has a single case, so every row in the results files shows Relative 1.0X and the committed numbers cannot show the win. Since the old implementation is gone, a baseline case inside the file is not possible; the simplest fix is to paste the before/after numbers already measured in the review thread (array_union on 100k elements: 49269ms to 1779ms) into the description's "Local benchmark results" section (or attach the results of the Run benchmarks action), which is currently empty.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark results got lost in the last PR description update, re-added

TypeUtils.getInterpretedOrdering(et)
// If the element type supports proper equals, we use the values directly for comparison,
// otherwise we use the generic comparable wrapper so all types support hash-based operations
@transient protected lazy val keyGenerator: (Any => Any) =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The description says "all array set-like expressions", but arrays_overlap is unchanged: it still splits on typeWithProperEquals and falls back to the O(n²) bruteForceEval for complex types, so two 10k-element struct arrays still mean about 10^8 comparisons. Since the thread agrees on a follow-up, could you add a scope note to the description (this PR covers the ArraySetLike implementors) and open a JIRA for the follow-up, so "all" does not overstate the coverage?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated title and created a JIRA for arrays_overlap https://issues.apache.org/jira/browse/SPARK-58943

(value: Any) => {
val key = keyGenerator(value)
if (!hs.contains(key)) {
if (arrayBuffer.size > ByteArrayMethods.MAX_ROUNDED_ARRAY_LENGTH) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After unifying the paths, array_distinct on complex element types now enforces the MAX_ROUNDED_ARRAY_LENGTH limit: an array with more than ~2^31 distinct elements used to return a result from the nested loop and now throws arrayFunctionWithElementsExceedLimitError. The boundary is unreachable in practice and matches what array_union and the codegen path already did, so no code change needed; just worth a clause in the description, since it currently claims "No user-facing change".

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the description

@KimahrimanKimahriman changed the title [SPARK-54698][SQL] Support hashing for all data types for array set like operations[SPARK-54698][SQL] Support hashing for all data types for ArraySetLike operationsAug 23, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@Kimahriman@asugranyes@LuciferYang@qlong