Skip to content

feat(pyamber): validate large binary URI - #8280

Open
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/validate-large-binary-uri
Open

feat(pyamber): validate large binary URI#8280
carloea2 wants to merge 1 commit into
apache:mainfrom
carloea2:fix/validate-large-binary-uri

Conversation

@carloea2

@carloea2 carloea2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Validate that a large-binary S3 reference contains both a bucket and an object key during construction, before any stream reaches S3.

Any related issues, documentation, discussions?

Closes #8279

How was this PR tested?

Added negative coverage for missing buckets and keys. Existing valid URI, generated URI, equality, hashing, and stream tests remain green.

C:\Users\carlo\texera\texera\.venv312\Scripts\python.exe -c "import sys,pytest; sys.path[:0]=[r'C:\Users\carlo\texera\texera-worktrees\investigate-bug75\amber\src\main\python',r'C:\Users\carlo\texera\texera\amber\src\main\python']; raise SystemExit(pytest.main([r'amber\src\test\python\core\models\type\test_large_binary.py',r'amber\src\test\python\pytexera\storage\test_large_binary_input_stream.py','-p','no:cacheprovider','-q']))"

C:\Users\carlo\texera\texera\.venv312\Scripts\ruff.exe check amber/src/main/python/core/models/type/large_binary.py amber/src/test/python/core/models/type/test_large_binary.py

C:\Users\carlo\texera\texera\.venv312\Scripts\ruff.exe format --check amber/src/main/python/core/models/type/large_binary.py amber/src/test/python/core/models/type/test_large_binary.py

All 23 tests passed. Ruff checks passed.

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

Generated-by: Codex

@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: @kunwp1
    You can notify them by mentioning @kunwp1 in a comment.

@Yicong-Huang Yicong-Huang added the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@github-actions github-actions Bot removed the release/v1.2 back porting to release/v1.2 label Aug 31, 2026
@github-actions
github-actions Bot requested a review from xuang7 August 31, 2026 08:20
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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.3 Not labeled automatically — none of the files this PR modifies exist on this branch (amber/src/main/python/core/models/type/large_binary.py, amber/src/test/python/core/models/type/test_large_binary.py). The fix may target code that isn't on this release, or the files were moved/renamed after the branch was cut. Please check and add release/v1.3 by hand if this fix should be backported here.
🚫 release/v1.2 Label was removed earlier (opt-out); not re-added. Re-add it by hand if this fix should be backported here after all.

Auto-label run.

@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 50321e4 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 382 0.233 25,083/35,429/35,429 us 🔴 +15.3% / 🔴 +127.4%
🔴 bs=100 sw=10 sl=64 799 0.488 120,929/166,604/166,604 us 🔴 +6.8% / 🔴 +55.4%
bs=1000 sw=10 sl=64 921 0.562 1,082,977/1,137,292/1,137,292 us ⚪ within ±5% / 🔴 -11.6%
Baseline details

Latest main 50321e4 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 382 tuples/sec 429 tuples/sec 787.98 tuples/sec -11.0% -51.5%
bs=10 sw=10 sl=64 MB/s 0.233 MB/s 0.262 MB/s 0.481 MB/s -11.1% -51.6%
bs=10 sw=10 sl=64 p50 25,083 us 23,686 us 12,593 us +5.9% +99.2%
bs=10 sw=10 sl=64 p95 35,429 us 30,720 us 15,579 us +15.3% +127.4%
bs=10 sw=10 sl=64 p99 35,429 us 30,720 us 18,786 us +15.3% +88.6%
bs=100 sw=10 sl=64 throughput 799 tuples/sec 797 tuples/sec 1,008 tuples/sec +0.3% -20.7%
bs=100 sw=10 sl=64 MB/s 0.488 MB/s 0.486 MB/s 0.615 MB/s +0.4% -20.7%
bs=100 sw=10 sl=64 p50 120,929 us 121,346 us 100,701 us -0.3% +20.1%
bs=100 sw=10 sl=64 p95 166,604 us 155,977 us 107,244 us +6.8% +55.4%
bs=100 sw=10 sl=64 p99 166,604 us 155,977 us 116,122 us +6.8% +43.5%
bs=1000 sw=10 sl=64 throughput 921 tuples/sec 931 tuples/sec 1,042 tuples/sec -1.1% -11.6%
bs=1000 sw=10 sl=64 MB/s 0.562 MB/s 0.568 MB/s 0.636 MB/s -1.1% -11.6%
bs=1000 sw=10 sl=64 p50 1,082,977 us 1,067,878 us 981,959 us +1.4% +10.3%
bs=1000 sw=10 sl=64 p95 1,137,292 us 1,127,918 us 1,023,080 us +0.8% +11.2%
bs=1000 sw=10 sl=64 p99 1,137,292 us 1,127,918 us 1,051,697 us +0.8% +8.1%
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,523.24,200,128000,382,0.233,25082.83,35428.80,35428.80
1,100,10,64,20,2502.04,2000,1280000,799,0.488,120929.18,166603.95,166603.95
2,1000,10,64,20,21707.88,20000,12800000,921,0.562,1082977.01,1137292.30,1137292.30

@carloea2 carloea2 changed the title fix(pyamber): validate large binary URI feat(pyamber): validate large binary URI Aug 31, 2026
@xuang7 xuang7 removed the fix label Sep 1, 2026
@xuang7
xuang7 removed their request for review September 1, 2026 01:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Large binary accepts incomplete S3 references

3 participants