Optimize set operations by walking through sets. #574
+6,852
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously set{Diff,Inter,Member} were walking through first set and
checking whether item exist in second using existsInSet, in total taking
`O(n * log(n))` time. Taking advantage of the fact that both arrays are
already ordered, in all functions walking through both arrays
simultaneously.
Tested with: `./mill bench.runRegressions bench/resources/sjsonnet_suite/*.jsonnet`
Before:
```
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setDiff.jsonnet avgt 1.237 ms/op
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setInter.jsonnet avgt 0.870 ms/op
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setUnion.jsonnet avgt 5.926 ms/op
```
After:
```
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setDiff.jsonnet avgt 0.897 ms/op
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setInter.jsonnet avgt 0.701 ms/op
[113] RegressionBenchmark.main bench/resources/sjsonnet_suite/setUnion.jsonnet avgt 1.520 ms/op
```
hauserx
force-pushed
the
set_diff_improv
branch
from
7148c36 to
cac1280
Compare
Contributor
Author
|
Ah, I see that the go-jsonnet tests contain sets for which keyF creates duplicates. Let me re-check how they are handling it in their implementation. |
JoshRosen
pushed a commit
to JoshRosen/sjsonnet
that referenced
this pull request
Dec 21, 2025
This replaces the previous O(n log n) and O(n * log m) implementations of setUnion, setInter, and setDiff with efficient O(n+m) merge-walk algorithms that exploit the fact that sets are already sorted. Changes: - setUnion: Changed from concat + sort + dedup to merge-walk (was O((n+m) log(n+m)), now O(n+m)) - setInter: Changed from iterate + binary search to merge-walk (was O(min(n,m) * log(max(n,m))), now O(n+m)) - setDiff: Changed from iterate + binary search to merge-walk (was O(n * log m), now O(n+m)) - Added applyKeyF helper function for cleaner key function handling - Added validateSet calls to setUnion for consistency with other ops - Pre-sized ArrayBuffer allocations for better memory efficiency The setMember function still uses binary search which is optimal for single-element lookups. These optimizations address the same performance issue as upstream PR databricks#574, but avoid the O(n²) bug in that PR's uniqArr implementation (which calls ArrayBuilder.result() on each iteration). All changes respect the throwErrorForInvalidSets setting.
stephenamar-db
requested changes
Dec 22, 2025
| }, | ||
| builtinWithDefaults("setUnion", "a" -> null, "b" -> null, "keyF" -> Val.False(dummyPos)) { | ||
| (args, pos, ev) => | ||
| val keyF = args(2) |
Collaborator
There was a problem hiding this comment.
The only ask I have is that you put this new behavior in setUnion behind a flag.
I'm concerned with code relying on the fact that this function will currently create a set from a non-set.
We will have to look at all our users of std.setUnion - validateSet might save us here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously
set{Diff,Inter,Member}functions were walking through first set and checking whether item exist in second usingexistsInSet. In total it takesO(n * log(n))time. Taking advantage of the fact that both arrays are already ordered, in all functions walking through both arrays simultaneously.Added benchmarks and tested with:
./mill bench.runRegressions bench/resources/sjsonnet_suite/*.jsonnetBefore:
After: