Repository navigation
Conversation
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8282 +/- ##
=========================================
Coverage 91.87% 91.87%
Complexity 5049 5049
=========================================
Files 1265 1265
Lines 54915 54915
Branches 6940 6940
=========================================
Hits 50453 50453
Misses 2769 2769
Partials 1693 1693
*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:
|
Contributor
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 385 | 0.235 | 24,836/38,885/38,885 us | 🔴 +20.8% / 🔴 +173.2% |
| 🔴 | bs=100 sw=10 sl=64 | 848 | 0.518 | 116,680/128,961/128,961 us | 🔴 +5.8% / 🔴 +33.7% |
| ⚪ | bs=1000 sw=10 sl=64 | 976 | 0.596 | 1,025,451/1,103,386/1,103,386 us | ⚪ within ±5% / 🔴 +21.2% |
Baseline details
Latest main 98c6337 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 385 tuples/sec | 446 tuples/sec | 879.16 tuples/sec | -13.7% | -56.2% |
| bs=10 sw=10 sl=64 | MB/s | 0.235 MB/s | 0.272 MB/s | 0.537 MB/s | -13.6% | -56.2% |
| bs=10 sw=10 sl=64 | p50 | 24,836 us | 20,568 us | 11,504 us | +20.8% | +115.9% |
| bs=10 sw=10 sl=64 | p95 | 38,885 us | 39,365 us | 14,234 us | -1.2% | +173.2% |
| bs=10 sw=10 sl=64 | p99 | 38,885 us | 39,365 us | 17,338 us | -1.2% | +124.3% |
| bs=100 sw=10 sl=64 | throughput | 848 tuples/sec | 884 tuples/sec | 1,147 tuples/sec | -4.1% | -26.1% |
| bs=100 sw=10 sl=64 | MB/s | 0.518 MB/s | 0.539 MB/s | 0.7 MB/s | -3.9% | -26.0% |
| bs=100 sw=10 sl=64 | p50 | 116,680 us | 113,023 us | 88,987 us | +3.2% | +31.1% |
| bs=100 sw=10 sl=64 | p95 | 128,961 us | 121,924 us | 96,473 us | +5.8% | +33.7% |
| bs=100 sw=10 sl=64 | p99 | 128,961 us | 121,924 us | 102,791 us | +5.8% | +25.5% |
| bs=1000 sw=10 sl=64 | throughput | 976 tuples/sec | 950 tuples/sec | 1,182 tuples/sec | +2.7% | -17.4% |
| bs=1000 sw=10 sl=64 | MB/s | 0.596 MB/s | 0.58 MB/s | 0.721 MB/s | +2.8% | -17.4% |
| bs=1000 sw=10 sl=64 | p50 | 1,025,451 us | 1,055,952 us | 872,751 us | -2.9% | +17.5% |
| bs=1000 sw=10 sl=64 | p95 | 1,103,386 us | 1,128,042 us | 910,151 us | -2.2% | +21.2% |
| bs=1000 sw=10 sl=64 | p99 | 1,103,386 us | 1,128,042 us | 941,479 us | -2.2% | +17.2% |
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,518.88,200,128000,385,0.235,24836.45,38885.23,38885.23
1,100,10,64,20,2357.71,2000,1280000,848,0.518,116680.43,128961.23,128961.23
2,1000,10,64,20,20498.50,20000,12800000,976,0.596,1025451.11,1103385.80,1103385.80
This was referenced Aug 31, 2026
renovate-bot
pushed a commit
to renovate-bot/apache-_-texera
that referenced
this pull request
Aug 31, 2026
### What changes were proposed in this PR? One bullet in `amber/LICENSE-binary-python`, in the BSD 3-Clause section: ``` - click==8.4.2 + - cloudpickle==3.1.2 - contourpy==1.3.3 ``` **Why it is a bug.** `build / pyamber (ubuntu-latest, 3.12)` has been red repo-wide since ~09:00-13:00 UTC on 2026-08-31, on every open PR regardless of what the PR touches, because **joblib 1.6.0 stopped vendoring cloudpickle**: | | joblib 1.5.3 | joblib 1.6.0 | | --- | --- | --- | | `joblib/externals/cloudpickle/` in the wheel | present (vendored) | **gone** | | `Requires-Dist` | *(none)* | `cloudpickle>=3.0` | | visible to `pip-licenses` | no | **yes** | `amber/operator-requirements.txt` reaches joblib transitively, so pip now installs cloudpickle as its own distribution and the manifest no longer describes what the image bundles: ``` operator-requirements.txt -> scikit-learn==1.7.2 -> joblib>=1.2.0 -> cloudpickle>=3.0 ``` ``` Before: any PR -> pyamber 3.12 -> "+ cloudpickle==3.1.2" -> red After: any PR -> pyamber 3.12 -> "OK: 110 Python packages match ..." -> green ``` Note that the code was always in the distribution -- it shipped inside the joblib wheel. What changed is that it became a separate distribution, and so became visible to the license scanner. **License clearance.** cloudpickle 3.1.2's wheel METADATA says `License: BSD-3-Clause` (PyPI's classifier reports only the generic `BSD License`, which is what the CI line quotes), and the bundled `LICENSE` is the canonical three-clause text. That is **ASF Category A**. The wheel ships no `NOTICE`, and `licenses/LICENSE-BSD-3-Clause.txt` already carries the text that the section references, so no `NOTICE-binary` / `NOTICE-binary-python` change is needed and the one bullet is the entire fix. **Deliberately out of scope.** The same step prints a long `DRIFT (transitive, informational)` list (`charset-normalizer`, `click`, `joblib`, `regex`, `scipy`, ...). PR runs pass `--ignore-transitive-version`, so none of it is fatal, and the nightly exact-match check on `main` owns refreshing those pins -- bulk-bumping 22 moving targets here would just add churn. That is why `joblib` stays at `1.5.3` in the manifest even though `1.6.0` is what introduced the coupling. This unblocks every open PR's `build / pyamber (ubuntu-latest, 3.12)` job. ### Any related issues, documentation, discussions? Closes apache#8292 Related: apache#8294 ### How was this PR tested? Reproduced the CI check locally, byte-for-byte, without a full `pip install`. CI's own report is a complete description of the diff between the manifest and reality (1 `+`, 0 stale, 0 direct drift, 22 transitive drifts), so the installed set was rebuilt as "every claimed bullet at its bundled version, plus cloudpickle==3.1.2" and fed to the checker exactly as `build.yml` invokes it: ```bash python bin/licensing/check_binary_deps.py --ignore-transitive-version python /tmp/pip-licenses.csv ``` | | exit | output | | --- | --- | --- | | before (main) | 1 | `+ cloudpickle==3.1.2 (license: BSD License)` -- identical to run 33394216939 | | after (this PR) | 0 | `OK: 110 Python packages match LICENSE-binary.` | The `--ignore-transitive-version`-less (nightly) mode still exits 1 on the 22 transitive drifts both before and after this change, so the behavior there is unchanged -- that is the nightly refresh's job, not this PR's. Upstream cause confirmed straight from the published wheels: ```bash pip download joblib==1.5.3 --no-deps -d . && pip download joblib==1.6.0 --no-deps -d . # 1.6.0 METADATA: Requires-Dist: cloudpickle>=3.0, and no joblib/externals/cloudpickle/ # 1.5.3 METADATA: no Requires-Dist, and joblib/externals/cloudpickle/ present pip download cloudpickle==3.1.2 --no-deps -d . # METADATA: License: BSD-3-Clause; licenses/LICENSE is the 3-clause text; no NOTICE ``` The failure was verified as repo-wide rather than PR-specific on two unrelated branches -- `chore/remove-superseded-sql-updates` (run 33394216939, 12:55 UTC) and `ci/8084-backport-manager-approval-gate` (run 33392828233, 12:39 UTC) -- both failing on the same `+ cloudpickle==3.1.2` line, against green license steps at 08:39 UTC (apache#8282) and 08:49 UTC (apache#8286). One wrinkle worth flagging for the reviewer: `build` was **skipped** on this PR's first runs. `amber/LICENSE-binary-python` matches no glob in `.github/labeler.yml`, so no stack label was applied, `precheck` selected no stacks, and the aggregate gate accepts `build: skipped` as passing -- i.e. a change to this manifest is not normally gated by the check that validates it. The `pyamber` label was applied by hand here so the license check actually runs against the fix. That gap is filed separately as apache#8294 and left out of this diff to keep the hotfix a one-liner. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5)
carloea2
changed the title
xuang7
removed their request for review
September 1, 2026 01:42
Contributor
Author
|
/request-review @Ma77Ball |
Contributor
Author
|
/request-review @Yicong-Huang |
Ma77Ball
approved these changes
Sep 11, 2026
Ma77Ball
left a comment
Ma77Ball
left a comment
Contributor
There was a problem hiding this comment.
LGTM! Reviewed statically. I noticed no blocking issues.
xuang7
pushed a commit
to xuang7/texera
that referenced
this pull request
Sep 18, 2026
### What changes were proposed in this PR? One bullet in `amber/LICENSE-binary-python`, in the BSD 3-Clause section: ``` - click==8.4.2 + - cloudpickle==3.1.2 - contourpy==1.3.3 ``` **Why it is a bug.** `build / pyamber (ubuntu-latest, 3.12)` has been red repo-wide since ~09:00-13:00 UTC on 2026-08-31, on every open PR regardless of what the PR touches, because **joblib 1.6.0 stopped vendoring cloudpickle**: | | joblib 1.5.3 | joblib 1.6.0 | | --- | --- | --- | | `joblib/externals/cloudpickle/` in the wheel | present (vendored) | **gone** | | `Requires-Dist` | *(none)* | `cloudpickle>=3.0` | | visible to `pip-licenses` | no | **yes** | `amber/operator-requirements.txt` reaches joblib transitively, so pip now installs cloudpickle as its own distribution and the manifest no longer describes what the image bundles: ``` operator-requirements.txt -> scikit-learn==1.7.2 -> joblib>=1.2.0 -> cloudpickle>=3.0 ``` ``` Before: any PR -> pyamber 3.12 -> "+ cloudpickle==3.1.2" -> red After: any PR -> pyamber 3.12 -> "OK: 110 Python packages match ..." -> green ``` Note that the code was always in the distribution -- it shipped inside the joblib wheel. What changed is that it became a separate distribution, and so became visible to the license scanner. **License clearance.** cloudpickle 3.1.2's wheel METADATA says `License: BSD-3-Clause` (PyPI's classifier reports only the generic `BSD License`, which is what the CI line quotes), and the bundled `LICENSE` is the canonical three-clause text. That is **ASF Category A**. The wheel ships no `NOTICE`, and `licenses/LICENSE-BSD-3-Clause.txt` already carries the text that the section references, so no `NOTICE-binary` / `NOTICE-binary-python` change is needed and the one bullet is the entire fix. **Deliberately out of scope.** The same step prints a long `DRIFT (transitive, informational)` list (`charset-normalizer`, `click`, `joblib`, `regex`, `scipy`, ...). PR runs pass `--ignore-transitive-version`, so none of it is fatal, and the nightly exact-match check on `main` owns refreshing those pins -- bulk-bumping 22 moving targets here would just add churn. That is why `joblib` stays at `1.5.3` in the manifest even though `1.6.0` is what introduced the coupling. This unblocks every open PR's `build / pyamber (ubuntu-latest, 3.12)` job. ### Any related issues, documentation, discussions? Closes apache#8292 Related: apache#8294 ### How was this PR tested? Reproduced the CI check locally, byte-for-byte, without a full `pip install`. CI's own report is a complete description of the diff between the manifest and reality (1 `+`, 0 stale, 0 direct drift, 22 transitive drifts), so the installed set was rebuilt as "every claimed bullet at its bundled version, plus cloudpickle==3.1.2" and fed to the checker exactly as `build.yml` invokes it: ```bash python bin/licensing/check_binary_deps.py --ignore-transitive-version python /tmp/pip-licenses.csv ``` | | exit | output | | --- | --- | --- | | before (main) | 1 | `+ cloudpickle==3.1.2 (license: BSD License)` -- identical to run 33394216939 | | after (this PR) | 0 | `OK: 110 Python packages match LICENSE-binary.` | The `--ignore-transitive-version`-less (nightly) mode still exits 1 on the 22 transitive drifts both before and after this change, so the behavior there is unchanged -- that is the nightly refresh's job, not this PR's. Upstream cause confirmed straight from the published wheels: ```bash pip download joblib==1.5.3 --no-deps -d . && pip download joblib==1.6.0 --no-deps -d . # 1.6.0 METADATA: Requires-Dist: cloudpickle>=3.0, and no joblib/externals/cloudpickle/ # 1.5.3 METADATA: no Requires-Dist, and joblib/externals/cloudpickle/ present pip download cloudpickle==3.1.2 --no-deps -d . # METADATA: License: BSD-3-Clause; licenses/LICENSE is the 3-clause text; no NOTICE ``` The failure was verified as repo-wide rather than PR-specific on two unrelated branches -- `chore/remove-superseded-sql-updates` (run 33394216939, 12:55 UTC) and `ci/8084-backport-manager-approval-gate` (run 33392828233, 12:39 UTC) -- both failing on the same `+ cloudpickle==3.1.2` line, against green license steps at 08:39 UTC (apache#8282) and 08:49 UTC (apache#8286). One wrinkle worth flagging for the reviewer: `build` was **skipped** on this PR's first runs. `amber/LICENSE-binary-python` matches no glob in `.github/labeler.yml`, so no stack label was applied, `precheck` selected no stacks, and the aggregate gate accepts `build: skipped` as passing -- i.e. a change to this manifest is not normally gated by the check that validates it. The `pyamber` label was applied by hand here so the license check actually runs against the fix. That gap is filed separately as apache#8294 and left out of this diff to keep the hotfix a one-liner. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Opus 5) (cherry picked from commit 267d916) (cherry picked from commit 68f9f30)
Yicong-Huang
left a comment
Yicong-Huang
left a comment
Contributor
There was a problem hiding this comment.
🟡 0 must-fix · 3 advisory · 1 polish: a correct, minimal, well-tested fix with nothing blocking it.
Design & architecture (1)
timed_buffer.py:39: a backward wall-clock step now suppresses the time-based flush (advisory, see inline)
Simplifications (1)
test_timed_buffer.py:132: the new boundary is pinned in the flushing direction only (advisory, see inline)
Conventions (1)
timed_buffer.py:37: every caller passesforce_flush=True, so the issue's symptom may not be reachable (advisory, question, see inline)
Polish: 1 quick touch-up (see inline comments).
Verification trace
Checked the two things the diff rests on rather than reading them off. The float comparison is exact at these magnitudes: timedelta(milliseconds=100).total_seconds() == 100/1000 is True. The new test is a real guard rather than a tautology, since the pre-PR expression evaluates 0 >= 0.5 at exactly 500 ms elapsed and fails it. The fix also silently repairs a second latent bug the description does not mention: .seconds drops the days component, so a 24-hour-old timestamp read as 0 and suppressed the flush entirely.
Reachability, which is what holds the two advisories below at advisory: all five call sites of MainLoop._check_and_report_console_messages pass force_flush=True, and flush is the first operand of the or, so no production path reaches the time branch today.
carloea2
force-pushed
the
fix/honor-subsecond-buffer-flush
branch
from
September 21, 2026 06:21
8df0da6 to
5c55c86
Compare
Contributor
Author
|
/request-review @Yicong-Huang |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
Contributor
There was a problem hiding this comment.
🟢 4 resolved · 0 open · 0 new
Verification trace
Checked the two things this round rests on rather than reading them off. The fake clock starts at 100.0 and both 100.5 and 0.5 are exactly representable, so the 500 ms flush case evaluates 0.5 >= 0.5 deterministically; the 400 ms withhold case evaluates 100.4 - 100.0 (0.40000000000000568) against 0.5 with a safe margin. The reset re-samples monotonic() after the comparison sample, and time.monotonic() is non-decreasing, so the re-armed interval can only run long, never short: no early flush is possible.
Contributor
Author
|
/request-review @sshiv012 @parshimers |
Ma77Ball
reviewed
Oct 21, 2026
carloea2
force-pushed
the
fix/honor-subsecond-buffer-flush
branch
from
October 24, 2026 23:52
5c55c86 to
a1ce37a
Compare
Contributor
Author
Yicong-Huang
left a comment
Yicong-Huang
left a comment
Contributor
There was a problem hiding this comment.
🟢 4 resolved · 0 open · 1 new (1 new = 1 newly introduced · 0 late catches)
Polish: 1 quick touch-up (see inline comments).
Verification trace
This round's delta is the rebase onto main plus the restored comment in test_elapsed_interval_triggers_flush. The three changed files were re-read in full: timed_buffer.py is identical to the version checked last round, and every MainLoop caller still passes force_flush=True, so production behavior is unchanged as the description states.
carloea2
force-pushed
the
fix/honor-subsecond-buffer-flush
branch
from
October 25, 2026 01:56
a1ce37a to
ec624ba
Compare
Contributor
Author
|
/request-review @Yicong-Huang The comment is corrected. All 21 focused tests and Ruff checks pass locally, and Python CI passes on all three versions. Integration CI is still running. Could you give the final review and merge once the required checks pass? |
Yicong-Huang
left a comment
Yicong-Huang
left a comment
Contributor
There was a problem hiding this comment.
🟢 1 resolved · 0 open · 0 new (0 new = 0 newly introduced · 0 late catches)
Verification trace
This round verified the last open item: the stale timedelta.seconds clause is gone from the test_elapsed_interval_triggers_flush comment, and the surviving wording matches the monotonic float comparison. The production helper is unchanged from the version checked last round, and every MainLoop caller still passes force_flush=True, so production behavior is unchanged as the description states.
Contributor
Author
|
/request-review @Yicong-Huang All CI is now green, including Ubuntu and macOS integration. Your latest review confirms zero open findings. Could you give the formal approval when you have a moment? |
This branch has not been deployed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What changes were proposed in this PR?
Use a monotonic clock for elapsed buffer time so subsecond intervals are honored and wall-clock changes cannot suppress a flush. Update the elapsed-interval comment to match the monotonic clock.
This changes the TimedBuffer helper. Current MainLoop callers force flushing, so this does not claim to change their production behavior.
Any related issues, documentation, discussions?
Closes #8281
How was this PR tested?
Fresh detached worktree from upstream main at
98c63374b7, Python 3.12 on Windows. All 21 focused tests pass, covering subsecond boundaries, backward wall-clock changes, timer resets, forced flushes, and size-based flushes.Generate protobufs with
bash bin/python-proto-gen.sh, then run fromamber:Both Ruff checks pass for all 217 Python files. Rechecked all 21 focused tests after the comment correction. The previous integration failure is the separately reproduced PyIceberg signer race, tracked in #9129; this PR does not include that storage change.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Codex