Skip to content

feat(workflow-operator): export Aggregate and the sort operators as Python - #8709

Draft
kz930 wants to merge 10 commits into
apache:mainfrom
kz930:feat/standalone-aggregate-and-sort
Draft

kz930 wants to merge 10 commits into
apache:mainfrom
kz930:feat/standalone-aggregate-and-sort

Conversation

@kz930

@kz930 kz930 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Aggregate and the sort family (Sort, Stable Merge Sort and Sort Partitions) implement StandaloneCodeGenerator. The sort family's row order is its contract, so LogicalOp gains orderSensitive, which those three override. Anything comparing two runs of an operator reads it to decide whether the rows have to arrive in the same order or only be the same rows.

Review then found five places where the block answered a question the engine answers differently. Aggregate summed an INTEGER column without the wrap Java ints have, refused a TIMESTAMP column outright, answered a timestamp average with a timestamp where AVERAGE is declared DOUBLE, and spelled a boolean in Python's capitals rather than Java's lower case. The sort put a NaN last on the way down, where the engine puts it first.

The edge values the shared table now carries found one more. Aggregate's concatenation wrote a double past 1e7 or under 1e-3, and a timestamp, as Python prints them, where the engine writes what Java's toString gives. It now writes every value, the lower-case boolean the review asked for included, through the helper #8597 adds for casting to text.

Any related issues, documentation, discussions?

Part of #8325, step 11b; that issue lists the set in order. Aggregate's concatenation calls a helper #8597 adds, so #8597 merges first.

Sorting a NaN apart from a null, and grouping on one, need the nullable dtypes #8704 reads an Arrow file into. Without it a null and a NaN share a slot and both behave as they did before, so this PR is no worse on its own and only complete alongside that one.

Closes #8707, the task this change is the whole of.

How was this PR tested?

Each operator asserts the block it emits in its own spec. The Aggregate cases the review named run in the verify tables of #8363, which put the same input through the engine and through the script and compare the two. What those tables cannot reach stays here as executed tests: a timestamp at two resolutions and in a zone other than UTC, and a null kept apart from a NaN in the sort and the group key.

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

Generated-by: Claude Code (Claude Opus 5)

🤖 Generated with Claude Code

…ython

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • Contributors with relevant context: @aglinxinyuan, @tanishqgandhi1908, @Yicong-Huang
    You can notify them by mentioning @aglinxinyuan, @tanishqgandhi1908, @Yicong-Huang in a comment.

@codecov-commenter

codecov-commenter commented Sep 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 21.64948% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.43%. Comparing base (5264df2) to head (99acd18).
⚠️ Report is 125 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...era/amber/operator/aggregate/AggregateOpDesc.scala 24.65% 50 Missing and 5 partials ⚠️
...ra/amber/operator/sort/StableMergeSortOpDesc.scala 8.33% 11 Missing ⚠️
...apache/texera/amber/operator/sort/SortOpDesc.scala 14.28% 6 Missing ⚠️
...operator/sortPartitions/SortPartitionsOpDesc.scala 25.00% 3 Missing ⚠️
...a/org/apache/texera/amber/operator/LogicalOp.scala 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (21.64%) 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    #8709      +/-   ##
============================================
- Coverage     92.78%   92.43%   -0.36%     
- Complexity     4899     5048     +149     
============================================
  Files          1236     1244       +8     
  Lines         52130    53108     +978     
  Branches       6407     6543     +136     
============================================
+ Hits          48369    49090     +721     
- Misses         2185     2382     +197     
- Partials       1576     1636      +60     
Flag Coverage Δ *Carryforward flag
access-control-service 77.38% <ø> (+5.59%) ⬆️
agent-service 99.32% <ø> (ø) Carriedforward from 996e149
amber 87.76% <21.64%> (-0.86%) ⬇️
computing-unit-managing-service 60.48% <ø> (+5.28%) ⬆️
config-service 87.37% <ø> (ø)
file-service 81.53% <ø> (ø)
frontend 96.58% <ø> (ø) Carriedforward from 996e149
notebook-migration-service 83.73% <ø> (ø)
pyamber 98.48% <ø> (ø) Carriedforward from 996e149
workflow-compiling-service 74.09% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

🟢 6 better · 🔴 2 worse · ⚪ 7 noise (<±5%) · 0 without baseline

Compared against main ba2667a benchmarked on this same runner, so the delta is largely free of cross-runner hardware noise. The "7d avg" column still reflects the gh-pages dashboard. Treat <±5% as noise unless repeated.

Dashboard · Run

config throughput MB/s latency max Δ latest / 7d
🟢 bs=10 sw=10 sl=64 375 0.229 26,730/33,161/33,161 us 🟢 -66.9% / 🔴 +126.0%
🟢 bs=100 sw=10 sl=64 802 0.489 121,814/139,982/139,982 us 🟢 -5.2% / 🔴 +36.8%
🔴 bs=1000 sw=10 sl=64 915 0.558 1,093,207/1,203,951/1,203,951 us 🔴 +5.1% / 🔴 +22.3%
Baseline details

Latest main ba2667a from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 375 tuples/sec 309 tuples/sec 866.99 tuples/sec +21.4% -56.7%
bs=10 sw=10 sl=64 MB/s 0.229 MB/s 0.189 MB/s 0.529 MB/s +21.2% -56.7%
bs=10 sw=10 sl=64 p50 26,730 us 27,099 us 11,989 us -1.4% +123.0%
bs=10 sw=10 sl=64 p95 33,161 us 100,297 us 14,672 us -66.9% +126.0%
bs=10 sw=10 sl=64 p99 33,161 us 100,297 us 17,906 us -66.9% +85.2%
bs=100 sw=10 sl=64 throughput 802 tuples/sec 792 tuples/sec 1,111 tuples/sec +1.3% -27.8%
bs=100 sw=10 sl=64 MB/s 0.489 MB/s 0.484 MB/s 0.678 MB/s +1.0% -27.9%
bs=100 sw=10 sl=64 p50 121,814 us 123,018 us 95,405 us -1.0% +27.7%
bs=100 sw=10 sl=64 p95 139,982 us 147,694 us 102,332 us -5.2% +36.8%
bs=100 sw=10 sl=64 p99 139,982 us 147,694 us 115,219 us -5.2% +21.5%
bs=1000 sw=10 sl=64 throughput 915 tuples/sec 925 tuples/sec 1,141 tuples/sec -1.1% -19.8%
bs=1000 sw=10 sl=64 MB/s 0.558 MB/s 0.564 MB/s 0.696 MB/s -1.1% -19.8%
bs=1000 sw=10 sl=64 p50 1,093,207 us 1,077,488 us 938,794 us +1.5% +16.4%
bs=1000 sw=10 sl=64 p95 1,203,951 us 1,145,258 us 984,521 us +5.1% +22.3%
bs=1000 sw=10 sl=64 p99 1,203,951 us 1,145,258 us 1,008,827 us +5.1% +19.3%
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,532.86,200,128000,375,0.229,26730.04,33161.48,33161.48
1,100,10,64,20,2494.31,2000,1280000,802,0.489,121814.33,139982.35,139982.35
2,1000,10,64,20,21858.24,20000,12800000,915,0.558,1093207.05,1203950.80,1203950.80

@kz930

kz930 commented Sep 27, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Split out of #8506 so each part reads in one sitting; the code is what you reviewed there, plus today's fixes.

@carloea2 carloea2 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.

One empty-input mismatch remains. Reproduced with the emitted aggregate fragment and pandas 2.2.3, then checked against AggregateOpExec.

…h no group keys

The engine opens a group only when a row arrives, so an empty input with no
group keys finishes with nothing to emit. The exported script built one row
anyway, for example a COUNT of 0. It now emits an empty frame with the result
columns, and a parity test runs both on zero rows.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930
kz930 requested a review from carloea2 October 8, 2026 07:54

@carloea2 carloea2 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.

The empty Aggregate case now passes. The 26 existing focused tests pass too, but a native versus exported string-sort probe found one remaining mismatch.

The engine orders strings with String.compareTo, which compares UTF-16
code units, while pandas compares code points. The two disagree once a
character above U+FFFF meets one between U+E000 and U+FFFF, so the
generated script now sorts a string key by its UTF-16 big-endian bytes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@carloea2 carloea2 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.

The Unicode sort finding is fixed. I ran all four focused specs: 27 tests pass, but a new native versus generated aggregate regression fails with a stored NaN.

…e engine does

A DOUBLE column read from Arrow keeps a stored NaN apart from null. The
engine skips only the nulls and folds the rest as Java doubles, so SUM,
AVERAGE and MAX answer NaN and MIN answers the smallest number. pandas'
reductions answered null instead, and grouped MIN and MAX skipped the NaN.
The exported script now writes the engine's folds out for a declared DOUBLE
column and keeps grouped results in a nullable column, so NaN and null stay
apart. A parity test runs both on the same rows, with and without group keys.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930

kz930 commented Oct 12, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Fixed the stored-NaN mismatch in 79be3d4, and MIN and MAX now match the engine too. Could you take another look?

@carloea2 carloea2 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.

The updated SUM, AVERAGE, MIN and MAX NaN parity test passes. All 28 focused tests and scalafmtCheckAll pass. A new CONCAT parity test still fails with the #8597 helper supplied; its finite-value and null control passes.

Read nulls from the column's mask instead of pd.isna on each value, which
took a stored NaN for a null too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930

kz930 commented Oct 13, 2026

Copy link
Copy Markdown
Contributor Author

@carloea2 Thanks for catching the CONCAT case. It now reads nulls from the column's mask and is covered in the NaN parity test (ed55981), could you take another look?

@carloea2 carloea2 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.

The CONCAT NaN fix passes. With the declared #8597 cast helper supplied locally, all 28 focused tests pass, plus my original grouped and scalar reproductions with separate NaN and null cells. Formatting passes. The branch alone still fails with _texera_java_text undefined, so it needs that prerequisite before merging.

sbt -batch 'WorkflowOperator/testOnly *AggregateOpDescSpec *StableMergeSortOpDescSpec *SortPartitionsOpDescSpec *SortOpDescSpec -- -oDF' 'scalafmtCheckAll'

kz930 and others added 5 commits October 13, 2026 13:31
…fuses them

Without a zone file, as on Windows, gettz() answers tzlocal(), which asks
localtime, and Windows refuses an instant before 1970. Reading the column
raised, and the sum's scalar tz_convert swallowed the refusal and answered
in UTC. Read a refused cell at the zone's standard offset, and convert the
total through datetime so the refusal reaches the same fallback.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the engine does

The engine keys a group on a List of the key fields in a HashMap, under
Scala's ==, so every NaN row opens a group of its own. groupby, and the merge
that joined the results back by key, put every NaN in one group. Group by
an id that follows the engine's equality instead.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tamps as the engine does

Double.compare puts -0.0 below 0.0, where pandas reads the two as equal,
so both sorts gain a sign tier for a double key. Sort Partitions compares a
timestamp by getTime, so two inside one millisecond keep their input
order, and compares strings by UTF-16 code unit as Stable Merge Sort does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…instant

gettz() falls back to tzlocal() where there is no zone file, as on Windows,
and tzlocal() knows only today's two offsets; on macOS and Linux the zone
file reads a wall clock before 1900 at local mean time, where
java.util.Date takes today's standard offset. Read the machine's zone by
its IANA name through the shared helpers, and keep the standard offset
before 1900 in both directions.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… engine does

AttributeTypeUtils.compare reads a timestamp by getTime, so two in the
same millisecond tie and the engine's fold keeps the one it met first,
with its own digits past the millisecond. pandas' min and max compared
the full resolution and picked the other one. Fold a declared TIMESTAMP
column by its floored milliseconds instead, grouped or not.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kz930
kz930 marked this pull request as draft October 15, 2026 22:58

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Export Aggregate and the sort operators as Python

3 participants