You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "eugenegujing (via GitHub)" <gi...@apache.org> on 2026/08/10 23:06:06 UTC
[PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
eugenegujing opened a new pull request, #7532:
URL: https://github.com/apache/texera/pull/7532
### What changes were proposed in this PR?
One-line fix in `AggregationOperation.scala`: the `max` aggregation starts its running maximum from the type's minimum value, so its final "was this group empty?" check must compare against that same minimum value, but it compared against the type's maximum value instead (the line was copied from `min`, where that comparison is correct because `min` starts from the maximum).
Because the check looked at the wrong sentinel, two results were silently wrong while the workflow completed with no error:
- A group with only null values returned the sentinel itself instead of `null`, e.g. `-2147483648` for INTEGER or `1970-01-01 00:00:00` for TIMESTAMP.
- A true maximum equal to the type's maximum value was mistaken for "no value seen" and discarded, so `max` over `{1, 5, 2147483647}` reported `5` (the largest value from the other local aggregation worker) instead of `2147483647`.
#### Before-and-after
Test data: group `g1` has only nulls, `g2` contains `{1, 5, 2147483647}`, `g3` contains `{10, 42}`.
<img width="1347" height="900" alt="Screenshot 2026-08-07 at 2 27 40 PM" src="https://github.com/user-attachments/assets/faac65fd-e83f-4952-a0dd-b8109d56334d" />
Before the fix, `max(v)` grouped by `k` returns `-2147483648` for `g1` (expected `null`) and `5` for `g2` (expected `2147483647`):
<img width="1341" height="866" alt="Screenshot 2026-08-07 at 2 27 51 PM" src="https://github.com/user-attachments/assets/fdf7f045-aa34-44eb-adee-6e722839d7eb" />
`min(v)` on the same data is correct (`null` for `g1`, `1` for `g2`, `10` for `g3`), confirming only `max`'s empty-group check is broken:
<img width="1342" height="868" alt="Screenshot 2026-08-07 at 2 28 00 PM" src="https://github.com/user-attachments/assets/3df9c390-626f-4f01-9ca6-7e634927386b" />
After the fix, `max(v)` returns `null` for `g1`, `2147483647` for `g2`, and `42` for `g3`:
<img width="1131" height="756" alt="Screenshot 2026-08-07 at 5 54 17 PM" src="https://github.com/user-attachments/assets/68ba757c-8b4b-4682-b04e-8db740d1583c" />
### Any related issues, documentation, discussions?
Closes #7531
The regression was introduced by #1840, which generalised the hard-coded `Double` sentinels to per-type `minValue`/`maxValue` helpers and updated `maxAgg`'s initialiser but not its finaliser.
### How was this PR tested?
Added 5 regression tests, all of which fail without the one-line fix and pass with it (verified in both directions):
- `AggregateOpSpec`: `max` over empty input and over all-null input returns `null`; `max` keeps a true maximum equal to the type's maximum value, covering INTEGER, LONG, DOUBLE, and TIMESTAMP.
- `AggregationOperationSpec`: a worker-to-final pipeline via `getFinal` keeps `Int.MaxValue` when partial results are re-aggregated, reproducing the two-worker scenario shown above; `max`'s merge stays neutral when one side saw no values.
Full aggregate suite: `sbt "WorkflowOperator/testOnly org.apache.texera.amber.operator.aggregate.*"` — 68 tests, all passing.
Also verified end to end in the UI with the workflow shown above (screenshots are from before and after rebuilding the backend with this fix).
### Was this PR authored or co-authored using generative AI tooling?
Co-authored by: Claude Code (Claude Fable 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5247049225
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7532?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 84.70%. Comparing base ([`5e41efa`](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`c879f75`](https://app.codecov.io/gh/apache/texera/commit/c879f75824d71f6edda138407dead729d4b6c6a2?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 58 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #7532 +/- ##
============================================
- Coverage 84.70% 84.70% -0.01%
Complexity 4153 4153
============================================
Files 1169 1169
Lines 46740 46739 -1
Branches 5202 5202
============================================
- Hits 39592 39591 -1
Misses 5433 5433
Partials 1715 1715
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.50% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `80.86% <ø> (-0.01%)` | :arrow_down: | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `50.72% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.97% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.05% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `86.45% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.89% <ø> (ø)` | | |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.55% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7532/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `26.31% <ø> (ø)` | | Carriedforward from [5e41efa](https://app.codecov.io/gh/apache/texera/commit/5e41efae2650c3e70d7490b15bb1fdf9b1983568?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7532?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5420720243
Backport PR opened: draft #7985 (https://github.com/apache/texera/pull/7985) to `release/v1.2`, assigned to @eugenegujing — needs manual work because the cherry-pick conflicts.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "aglinxinyuan (via GitHub)" <gi...@apache.org>.
aglinxinyuan merged PR #7532:
URL: https://github.com/apache/texera/pull/7532
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5247064125
<!-- texera-benchmarks-comment -->
## ⚠️ Benchmark changes need a look
🟢 0 better · 🔴 3 worse · ⚪ 12 noise (<±5%) · 0 without baseline
> Compared against main `133da7b` 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](https://apache.github.io/texera/dev/bench/) · [Run](https://github.com/apache/texera/actions/runs/31441024986)
| | config | throughput | MB/s | latency | max Δ latest / 7d |
|---|---|---:|---:|---:|---:|
| ⚪ | bs=10 sw=10 sl=64 | 503 | 0.307 | 18,397/29,294/29,294 us | ⚪ within ±5% / 🔴 +76.3% |
| 🔴 | bs=100 sw=10 sl=64 | 1,040 | 0.635 | 95,296/119,124/119,124 us | 🔴 +9.9% / 🟢 +10.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 1,250 | 0.763 | 807,214/861,629/861,629 us | ⚪ within ±5% / 🟢 +29.4% |
<details><summary>Baseline details</summary>
Latest main `133da7b` from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---:|---:|---:|---:|---:|
| bs=10 sw=10 sl=64 | throughput | 503 tuples/sec | 525 tuples/sec | 740.3 tuples/sec | -4.2% | -32.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.307 MB/s | 0.321 MB/s | 0.452 MB/s | -4.4% | -32.1% |
| bs=10 sw=10 sl=64 | p50 | 18,397 us | 18,108 us | 13,173 us | +1.6% | +39.7% |
| bs=10 sw=10 sl=64 | p95 | 29,294 us | 27,921 us | 16,615 us | +4.9% | +76.3% |
| bs=10 sw=10 sl=64 | p99 | 29,294 us | 27,921 us | 19,272 us | +4.9% | +52.0% |
| bs=100 sw=10 sl=64 | throughput | 1,040 tuples/sec | 1,093 tuples/sec | 939.86 tuples/sec | -4.8% | +10.7% |
| bs=100 sw=10 sl=64 | MB/s | 0.635 MB/s | 0.667 MB/s | 0.574 MB/s | -4.8% | +10.7% |
| bs=100 sw=10 sl=64 | p50 | 95,296 us | 90,323 us | 106,062 us | +5.5% | -10.2% |
| bs=100 sw=10 sl=64 | p95 | 119,124 us | 108,354 us | 113,212 us | +9.9% | +5.2% |
| bs=100 sw=10 sl=64 | p99 | 119,124 us | 108,354 us | 123,853 us | +9.9% | -3.8% |
| bs=1000 sw=10 sl=64 | throughput | 1,250 tuples/sec | 1,253 tuples/sec | 966.11 tuples/sec | -0.2% | +29.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.763 MB/s | 0.765 MB/s | 0.59 MB/s | -0.3% | +29.4% |
| bs=1000 sw=10 sl=64 | p50 | 807,214 us | 794,502 us | 1,038,733 us | +1.6% | -22.3% |
| bs=1000 sw=10 sl=64 | p95 | 861,629 us | 885,223 us | 1,085,289 us | -2.7% | -20.6% |
| bs=1000 sw=10 sl=64 | p99 | 861,629 us | 885,223 us | 1,115,555 us | -2.7% | -22.8% |
</details>
<details><summary>Raw CSV</summary>
```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,397.51,200,128000,503,0.307,18396.94,29294.22,29294.22
1,100,10,64,20,1922.23,2000,1280000,1040,0.635,95296.05,119124.20,119124.20
2,1000,10,64,20,16000.08,20000,12800000,1250,0.763,807214.20,861629.15,861629.15
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5247025618
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- **Contributors with relevant context:** `@tanishqgandhi1908`, `@aglinxinyuan`
You can notify them by mentioning `@tanishqgandhi1908`, `@aglinxinyuan` in a comment.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "eugenegujing (via GitHub)" <gi...@apache.org>.
eugenegujing commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5287768369
/request-review @aglinxinyuan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "eugenegujing (via GitHub)" <gi...@apache.org>.
eugenegujing commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5418198794
@aglinxinyuan PTAL!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow-operator): MAX aggregation checks the wrong empty-group sentinel [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7532:
URL: https://github.com/apache/texera/pull/7532#issuecomment-5247025311
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. `release/*` labels drive the post-merge backport, so add or remove one to change where this fix lands.
| Release branch | Analysis |
| --- | --- |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/31441011067)._
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org