Repository navigation
feat(workflow-operator): keep the rows Distinct's engine keeps for NaN and signed zero - #8975
Conversation
… and signed zero The export called drop_duplicates, which counts two NaN values as equal and 0.0 as equal to -0.0. The engine's LinkedHashSet compares rows with Scala's ==, under which NaN equals nothing, and hashes doubles by their bits, so it keeps all of them. Select rows by the key the set operators already use. Closes apache#8974 Co-Authored-By: Claude Opus 5.5 <[email protected]>
Automated Reviewer SuggestionsBased on the
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #8975 +/- ##
============================================
- Coverage 91.79% 91.78% -0.01%
+ Complexity 5020 5018 -2
============================================
Files 1274 1274
Lines 56301 56302 +1
Branches 7352 7352
============================================
- Hits 51682 51679 -3
- Misses 2766 2768 +2
- Partials 1853 1855 +2
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 549 | 0.335 | 17,598/25,036/25,036 us | 🔴 +22.4% / 🔴 +89.2% |
| 🟢 | bs=100 sw=10 sl=64 | 1,262 | 0.77 | 78,779/87,453/87,453 us | 🟢 -8.2% / 🟢 -17.6% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,487 | 0.907 | 673,469/730,292/730,292 us | ⚪ within ±5% / 🟢 -20.9% |
Baseline details
Latest main 2f0089f from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 549 tuples/sec | 622 tuples/sec | 929.71 tuples/sec | -11.7% | -40.9% |
| bs=10 sw=10 sl=64 | MB/s | 0.335 MB/s | 0.38 MB/s | 0.567 MB/s | -11.8% | -41.0% |
| bs=10 sw=10 sl=64 | p50 | 17,598 us | 14,375 us | 10,919 us | +22.4% | +61.2% |
| bs=10 sw=10 sl=64 | p95 | 25,036 us | 23,104 us | 13,231 us | +8.4% | +89.2% |
| bs=10 sw=10 sl=64 | p99 | 25,036 us | 23,104 us | 16,777 us | +8.4% | +49.2% |
| bs=100 sw=10 sl=64 | throughput | 1,262 tuples/sec | 1,262 tuples/sec | 1,209 tuples/sec | 0.0% | +4.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.77 MB/s | 0.77 MB/s | 0.738 MB/s | 0.0% | +4.4% |
| bs=100 sw=10 sl=64 | p50 | 78,779 us | 76,958 us | 86,081 us | +2.4% | -8.5% |
| bs=100 sw=10 sl=64 | p95 | 87,453 us | 95,251 us | 91,913 us | -8.2% | -4.9% |
| bs=100 sw=10 sl=64 | p99 | 87,453 us | 95,251 us | 106,071 us | -8.2% | -17.6% |
| bs=1000 sw=10 sl=64 | throughput | 1,487 tuples/sec | 1,497 tuples/sec | 1,244 tuples/sec | -0.7% | +19.5% |
| bs=1000 sw=10 sl=64 | MB/s | 0.907 MB/s | 0.914 MB/s | 0.76 MB/s | -0.8% | +19.4% |
| bs=1000 sw=10 sl=64 | p50 | 673,469 us | 669,299 us | 838,767 us | +0.6% | -19.7% |
| bs=1000 sw=10 sl=64 | p95 | 730,292 us | 704,273 us | 889,385 us | +3.7% | -17.9% |
| bs=1000 sw=10 sl=64 | p99 | 730,292 us | 704,273 us | 923,616 us | +3.7% | -20.9% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,364.05,200,128000,549,0.335,17597.67,25035.76,25035.76
1,100,10,64,20,1584.72,2000,1280000,1262,0.770,78779.13,87452.76,87452.76
2,1000,10,64,20,13452.78,20000,12800000,1487,0.907,673468.60,730291.59,730291.59
… served script Distinct now keeps the rows the engine's HashSet keeps through _texera_set_select rather than drop_duplicates, so the export endpoint's spec looks for that call. Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@carloea2 A friendly ping on this one; I've since added a test expecting Distinct's set select in the export. |
carloea2
left a comment
carloea2
left a comment
There was a problem hiding this comment.
DistinctOpDescSpec passes all 8 tests with the declared SetRowKeys helper supplied. WorkflowToPythonResourceSpec passes all 4 tests, and scalafmtCheckAll passes. NaN, null and signed-zero cases match the native distinct behavior. Keep the draft dependency on #8506.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
@carloea2 A gentle ping: since your approval, Distinct reads its rows through the shared |
carloea2
left a comment
carloea2
left a comment
There was a problem hiding this comment.
Retested against current main. All 8 focused Scala tests and 20 comparisons between the native executor and generated Python passed, including NaN, signed zero, nulls and mixed keys. CI is green.
|
The latest head passes the local retest and CI is green. @aglinxinyuan, could you review this when you have a chance? |
What changes were proposed in this PR?
Distinct's exported Python called
drop_duplicates, which counts two NaN values as equal and 0.0 as equal to -0.0. The engine'sDistinctOpExeckeeps rows in aLinkedHashSetthat compares them with Scala's==, under which NaN equals nothing, and hashes each double by its bits, so 0.0 and -0.0 land apart. The export therefore emitted fewer rows than the run it came from.The export now selects rows with
_texera_key_codes(in1df, signed=True), the shared helper from #8341: one code per row, equal exactly where the engine's rows are, soduplicated()keeps the first row of each key in input order.Any related issues, documentation, discussions?
Closes #8974.
How was this PR tested?
DistinctOpDescSpecgains a test that runsDistinctOpExecand the exported Python over 1.0, NaN, 0.0, -0.0, null, 1.0, NaN, null, -0.0 and compares the rows in order. The engine keeps 6. The old export kept 4 and fails the test. It replaces the test that pinned the olddrop_duplicatesline. All 8 Distinct tests pass.WorkflowToPythonResourceSpec, which looked fordrop_duplicatesin the served script, now looks for_texera_key_codes.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 5.5)
🤖 Generated with Claude Code