Skip to content

test(pyamber): add unit tests for IcebergTableWriter - #7579

Merged
aglinxinyuan merged 1 commit into
apache:mainfrom
eugenegujing:test/iceberg-table-writer
Aug 13, 2026
Merged

test(pyamber): add unit tests for IcebergTableWriter#7579
aglinxinyuan merged 1 commit into
apache:mainfrom
eugenegujing:test/iceberg-table-writer

Conversation

@eugenegujing

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

This PR adds amber/src/test/python/core/storage/iceberg/test_iceberg_table_writer.py with 14 pure unit tests using mocked catalog/table objects (no real Iceberg catalog, no Postgres, no network). Covered behaviors:

  • Constructor: loads the table via catalog.load_table(f"{namespace}.{name}") and takes buffer_size from StorageConfig.ICEBERG_TABLE_COMMIT_BATCH_SIZE.
  • Buffer-threshold flush: put_one below the threshold does not flush; reaching buffer_size triggers a flush (serde called with schema + buffered items, table.append called with serde's result, buffer cleared); items added after a flush start a fresh buffer.
  • close() flushes the remaining items when the buffer is non-empty, and performs no append when the buffer is empty.
  • open() clears a previously dirty buffer.
  • remove_one() removes a buffered item; removing an item already flushed out of the buffer raises ValueError (pinning the current list.remove behavior).
  • _flush_buffer() returns early on an empty buffer (no serde/append/refresh calls).
  • Retry path: table.append raising pyiceberg's CommitFailedException twice then succeeding completes the flush, with table.refresh() called once per attempt; a permanent failure is reraised after 10 attempts and the buffer is NOT cleared.
  • A serde failure propagates without touching the table: serde runs outside the retry loop, so it is called exactly once, no refresh/append happens, and the buffer is kept.
  • The retry decorator sets no exception filter, so a non-conflict error (e.g. ValueError) is also retried for all 10 attempts before being reraised (pinning the current behavior).

The retry tests patch tenacity.nap.time.sleep, so the exponential-backoff waits (wait_random_exponential(0.001, 10)) never sleep for real; the whole file runs in about a second.

No production code is changed.

Any related issues, documentation, discussions?

Closes #7575

How was this PR tested?

This PR is itself test-only. The new spec was run with:

cd amber && pytest src/test/python/core/storage/iceberg/test_iceberg_table_writer.py -v

Result: 14 passed in about a second. The file is formatted with black (unchanged by --check). The suite was mutation-checked: targeted mutations of the writer (flipping the >= threshold to >, dropping the buffer.clear() after append, removing table.refresh() in the retry body, making close() skip the flush, removing the empty-buffer early return, and lowering stop_after_attempt) each caused at least one test to fail, and the source was restored afterwards.

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

Co-authored by: Claude Code (Claude Fable 5)

Add mocked-catalog unit tests covering buffer-threshold flushing, close/open/remove_one semantics, and the tenacity commit-retry paths.
@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.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.89%. Comparing base (e878df3) to head (dcf9ab8).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #7579   +/-   ##
=========================================
  Coverage     87.88%   87.89%           
  Complexity     4276     4276           
=========================================
  Files          1176     1176           
  Lines         47018    47018           
  Branches       5245     5245           
=========================================
+ Hits          41323    41325    +2     
+ Misses         3972     3970    -2     
  Partials       1723     1723           
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from e878df3
agent-service 98.62% <ø> (ø) Carriedforward from e878df3
amber 82.88% <ø> (ø) Carriedforward from e878df3
computing-unit-managing-service 60.38% <ø> (ø) Carriedforward from e878df3
config-service 65.97% <ø> (ø) Carriedforward from e878df3
file-service 69.05% <ø> (ø) Carriedforward from e878df3
frontend 89.94% <ø> (ø) Carriedforward from e878df3
notebook-migration-service 78.89% <ø> (ø) Carriedforward from e878df3
pyamber 97.56% <ø> (+0.04%) ⬆️
workflow-compiling-service 26.31% <ø> (ø) Carriedforward from e878df3

*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

Copy link
Copy Markdown
Contributor

✅ No material benchmark regressions detected

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

Compared against main e878df3 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 407 0.248 23,800/33,677/33,677 us 🟢 -16.1% / 🔴 +110.6%
🟢 bs=100 sw=10 sl=64 803 0.49 124,429/142,300/142,300 us 🟢 -7.1% / 🔴 +31.7%
bs=1000 sw=10 sl=64 906 0.553 1,107,821/1,156,913/1,156,913 us ⚪ within ±5% / 🔴 -12.0%
Baseline details

Latest main e878df3 from same runner

config metric PR latest main 7d avg Δ latest Δ 7d
bs=10 sw=10 sl=64 throughput 407 tuples/sec 364 tuples/sec 784.3 tuples/sec +11.8% -48.1%
bs=10 sw=10 sl=64 MB/s 0.248 MB/s 0.222 MB/s 0.479 MB/s +11.7% -48.2%
bs=10 sw=10 sl=64 p50 23,800 us 26,433 us 12,590 us -10.0% +89.0%
bs=10 sw=10 sl=64 p95 33,677 us 40,133 us 15,991 us -16.1% +110.6%
bs=10 sw=10 sl=64 p99 33,677 us 40,133 us 18,694 us -16.1% +80.1%
bs=100 sw=10 sl=64 throughput 803 tuples/sec 803 tuples/sec 1,002 tuples/sec 0.0% -19.9%
bs=100 sw=10 sl=64 MB/s 0.49 MB/s 0.49 MB/s 0.612 MB/s 0.0% -19.9%
bs=100 sw=10 sl=64 p50 124,429 us 120,026 us 101,285 us +3.7% +22.9%
bs=100 sw=10 sl=64 p95 142,300 us 153,245 us 108,068 us -7.1% +31.7%
bs=100 sw=10 sl=64 p99 142,300 us 153,245 us 118,235 us -7.1% +20.4%
bs=1000 sw=10 sl=64 throughput 906 tuples/sec 914 tuples/sec 1,030 tuples/sec -0.9% -12.0%
bs=1000 sw=10 sl=64 MB/s 0.553 MB/s 0.558 MB/s 0.629 MB/s -0.9% -12.0%
bs=1000 sw=10 sl=64 p50 1,107,821 us 1,095,039 us 991,882 us +1.2% +11.7%
bs=1000 sw=10 sl=64 p95 1,156,913 us 1,151,793 us 1,038,496 us +0.4% +11.4%
bs=1000 sw=10 sl=64 p99 1,156,913 us 1,151,793 us 1,068,265 us +0.4% +8.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,491.82,200,128000,407,0.248,23800.27,33676.64,33676.64
1,100,10,64,20,2489.88,2000,1280000,803,0.490,124429.26,142300.40,142300.40
2,1000,10,64,20,22065.34,20000,12800000,906,0.553,1107821.12,1156913.28,1156913.28

@eugenegujing

Copy link
Copy Markdown
Contributor Author

/request-review @aglinxinyuan

@github-actions
github-actions Bot requested a review from aglinxinyuan August 13, 2026 03:55
@aglinxinyuan
aglinxinyuan added this pull request to the merge queue Aug 13, 2026
Merged via the queue into apache:main with commit 6f56024 Aug 13, 2026
26 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 the Python IcebergTableWriter

3 participants