Skip to content

test(pyamber): add unit tests for port_storage_writer - #6673

Merged
aglinxinyuan merged 1 commit into
apache:mainfrom
Ma77Ball:test/port-storage-writer-py-spec
Jul 22, 2026
Merged

test(pyamber): add unit tests for port_storage_writer#6673
aglinxinyuan merged 1 commit into
apache:mainfrom
Ma77Ball:test/port-storage-writer-py-spec

Conversation

@Ma77Ball

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

  • Add unit tests for PortStorageWriter.
  • Cover receive() dispatching PortStorageWriterElement.data_tuple to buffered_item_writer.put_one and raising TypeError on other elements.
  • Cover pre_start() and post_stop() delegating to the writer open() and close().

Any related issues, documentation, discussions?

Closes: #6672

How was this PR tested?

  • Run: cd amber && python -m pytest src/test/python/core/storage/runnables/test_port_storage_writer.py, expect all 5 tests passing.
  • Test-only change; no production code is modified.

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

Co-authored with Claude Opus 4.8 in compliance with ASF

@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:

  • No candidates found from git blame history.

@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Benchmark changes need a look

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

Compared against main 0467c76 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 560 0.342 17,272/23,324/23,324 us 🔴 -11.9% / 🔴 +49.2%
🔴 bs=100 sw=10 sl=64 1,203 0.734 82,220/99,713/99,713 us 🔴 +6.8% / 🟢 +20.1%
bs=1000 sw=10 sl=64 1,432 0.874 695,257/748,700/748,700 us ⚪ within ±5% / 🟢 +38.6%
Baseline details

Latest main 0467c76 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 560 tuples/sec 636 tuples/sec 767.23 tuples/sec -11.9% -27.0%
bs=10 sw=10 sl=64 MB/s 0.342 MB/s 0.388 MB/s 0.468 MB/s -11.9% -27.0%
bs=10 sw=10 sl=64 p50 17,272 us 15,838 us 12,705 us +9.1% +35.9%
bs=10 sw=10 sl=64 p95 23,324 us 22,940 us 15,633 us +1.7% +49.2%
bs=10 sw=10 sl=64 p99 23,324 us 22,940 us 20,214 us +1.7% +15.4%
bs=100 sw=10 sl=64 throughput 1,203 tuples/sec 1,261 tuples/sec 1,001 tuples/sec -4.6% +20.1%
bs=100 sw=10 sl=64 MB/s 0.734 MB/s 0.77 MB/s 0.611 MB/s -4.7% +20.1%
bs=100 sw=10 sl=64 p50 82,220 us 79,699 us 99,949 us +3.2% -17.7%
bs=100 sw=10 sl=64 p95 99,713 us 93,327 us 105,996 us +6.8% -5.9%
bs=100 sw=10 sl=64 p99 99,713 us 93,327 us 117,999 us +6.8% -15.5%
bs=1000 sw=10 sl=64 throughput 1,432 tuples/sec 1,420 tuples/sec 1,033 tuples/sec +0.8% +38.6%
bs=1000 sw=10 sl=64 MB/s 0.874 MB/s 0.867 MB/s 0.631 MB/s +0.8% +38.6%
bs=1000 sw=10 sl=64 p50 695,257 us 708,103 us 974,571 us -1.8% -28.7%
bs=1000 sw=10 sl=64 p95 748,700 us 734,565 us 1,016,467 us +1.9% -26.3%
bs=1000 sw=10 sl=64 p99 748,700 us 734,565 us 1,048,798 us +1.9% -28.6%
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,357.14,200,128000,560,0.342,17271.65,23323.63,23323.63
1,100,10,64,20,1662.70,2000,1280000,1203,0.734,82219.61,99712.82,99712.82
2,1000,10,64,20,13966.06,20000,12800000,1432,0.874,695257.15,748700.49,748700.49

Copilot AI 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.

Pull request overview

This PR adds Python unit tests in Amber (pyamber) to improve coverage for the PortStorageWriter runnable, specifically verifying its receive() dispatch behavior and its lifecycle hooks (pre_start()/post_stop()) delegating to the underlying BufferedItemWriter.

Changes:

  • Added tests asserting PortStorageWriter.receive() forwards PortStorageWriterElement.data_tuple to buffered_item_writer.put_one(...).
  • Added tests asserting receive() raises TypeError for non-PortStorageWriterElement queue entries and does not write anything.
  • Added tests asserting pre_start() calls open() and post_stop() calls close() on the wrapped writer.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

LGTM!

@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Jul 22, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 22, 2026
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Jul 22, 2026
Merged via the queue into apache:main with commit 998899f Jul 22, 2026
24 checks passed
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.

Add unit test coverage for port_storage_writer

3 participants