test(pyamber): cover IcebergDocument uri, count and clear paths - #8048
Merged
aglinxinyuan merged 1 commit intoAug 28, 2026
Merged
Conversation
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 #8048 +/- ##
============================================
+ Coverage 93.41% 93.43% +0.01%
Complexity 4696 4696
============================================
Files 1179 1179
Lines 47713 47713
Branches 5314 5314
============================================
+ Hits 44573 44579 +6
+ Misses 1698 1692 -6
Partials 1442 1442
*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 | 375 | 0.229 | 26,359/32,908/32,908 us | 🔴 -13.9% / 🔴 +137.7% |
| 🔴 | bs=100 sw=10 sl=64 | 786 | 0.48 | 124,913/152,175/152,175 us | 🔴 +13.8% / 🔴 +62.5% |
| ⚪ | bs=1000 sw=10 sl=64 | 934 | 0.57 | 1,072,986/1,128,683/1,128,683 us | ⚪ within ±5% / 🔴 +25.0% |
Baseline details
Latest main b01b11f from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 375 tuples/sec | 435 tuples/sec | 894.86 tuples/sec | -13.8% | -58.1% |
| bs=10 sw=10 sl=64 | MB/s | 0.229 MB/s | 0.266 MB/s | 0.546 MB/s | -13.9% | -58.1% |
| bs=10 sw=10 sl=64 | p50 | 26,359 us | 23,251 us | 11,139 us | +13.4% | +136.6% |
| bs=10 sw=10 sl=64 | p95 | 32,908 us | 30,410 us | 13,843 us | +8.2% | +137.7% |
| bs=10 sw=10 sl=64 | p99 | 32,908 us | 30,410 us | 16,839 us | +8.2% | +95.4% |
| bs=100 sw=10 sl=64 | throughput | 786 tuples/sec | 823 tuples/sec | 1,166 tuples/sec | -4.5% | -32.6% |
| bs=100 sw=10 sl=64 | MB/s | 0.48 MB/s | 0.502 MB/s | 0.711 MB/s | -4.4% | -32.5% |
| bs=100 sw=10 sl=64 | p50 | 124,913 us | 122,430 us | 87,509 us | +2.0% | +42.7% |
| bs=100 sw=10 sl=64 | p95 | 152,175 us | 133,727 us | 93,675 us | +13.8% | +62.5% |
| bs=100 sw=10 sl=64 | p99 | 152,175 us | 133,727 us | 102,153 us | +13.8% | +49.0% |
| bs=1000 sw=10 sl=64 | throughput | 934 tuples/sec | 926 tuples/sec | 1,198 tuples/sec | +0.9% | -22.0% |
| bs=1000 sw=10 sl=64 | MB/s | 0.57 MB/s | 0.565 MB/s | 0.731 MB/s | +0.9% | -22.1% |
| bs=1000 sw=10 sl=64 | p50 | 1,072,986 us | 1,081,819 us | 859,766 us | -0.8% | +24.8% |
| bs=1000 sw=10 sl=64 | p95 | 1,128,683 us | 1,182,283 us | 902,897 us | -4.5% | +25.0% |
| bs=1000 sw=10 sl=64 | p99 | 1,128,683 us | 1,182,283 us | 937,957 us | -4.5% | +20.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,533.40,200,128000,375,0.229,26359.44,32907.69,32907.69
1,100,10,64,20,2544.40,2000,1280000,786,0.480,124912.85,152174.84,152174.84
2,1000,10,64,20,21418.58,20000,12800000,934,0.570,1072985.59,1128683.43,1128683.43
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?
A new
TestIcebergDocumentWithMockCatalogclass intest_iceberg_document.py, taking the file from 9 collected items to 22.iceberg_document.pyreaches 100% on both lines and branches. +6 fully-covered lines today, +9 after #8040 — lines 90, 108 and 189 are50% (1/2)partials, invisible to the current branch-less report.The nine lines were exactly {79, 80, 81, 84, 90, 108, 109, 189, 190}:
get_uri,get_count,clear, and theget_afterskip guard.The measurement problem, and how it was handled
The existing
TestIcebergDocumentneeds a live postgres Iceberg catalog. CI supplies one; on this machine thetexerarole cannot readiceberg_tables, so all 9 of those tests fail locally and a raw local coverage run under-reports the file badly — enough to suggest ~50 winnable lines instead of 9.A throwaway sqlite-backed
SqlCatalogproxy was used to reproduce the same code paths, applied identically to both sides and then deleted. The honest gap is 9.The new tests need none of that. They patch
IcebergCatalogInstance.get_instancerather than mutating it, so they touch no process-global state and need no postgres, filesystem or network. They run in 1.25s.Two hazards found in the existing fixture
StorageConfig.initializeis a process-wide one-shot that raises on a second call, andtest_iceberg_document.py:45calls it unguarded at module level. That works only because this module happens to be the first module-level initializer imported — two sibling modules guard withif not StorageConfig._initialized. A new test module initializing at module level and sorting before it would break collection for the whole suite. Appending a class to the existing file avoids this; a separate file would have been a trap.IcebergCatalogInstance._catalogsis a class-level dict andreplace_instancemutates it for the rest of the session, poisoning every later suite in the shared process. The new tests sidestep it by patchingget_instance.Honest discounts
get_uriandget_counthave zero callers anywhere insrc/main/python, andclearhas none either — the Scala side has its own implementation. These are real statements with real logic, not@abstractmethodstubs, but the value is interface-contract conformance rather than live behaviour.from_index.num_of_skipped_recordsis provably 0 when the guard first runs, so the only route isget_after(-1). It is drivable through the public API, but no production caller produces a negative offset — a defensive-guard test. Counted strictly, the bundle is 7 lines and 4 arms.sum(f.file.record_count ...)tosum(1 ...)at line 110, killed only by the pre-existing postgres-gatedtest_get_counts. Line 110 is already covered in CI so it is not a coverage gap, but the kill is not this bundle's and one was not manufactured.Verification
Two reviewers returned seven findings; all repaired. The repair pass added zero coverage lines and zero arms — the builder's bundle already reached 124/124. Its entire value was mutation strength: 14 mutants that passed the delivered spec now fail. Stated plainly rather than sold as more coverage.
Full pyamber suite:
5 failed, 1172 passed, 7 errorsagainst main's1159 passed. Failure sets sorted and diffed — identical by identity, 12 entries.ruff checkandruff format --checkpass on CI's scope. The throwaway proxy is deleted andgit status --porcelainis clean.No production file is touched;
git diff -- 'amber/src/main/*'is empty.Any related issues, documentation, discussions?
Closes #8044
How was this PR tested?
The 9 pre-existing
TestIcebergDocumenttests still require the postgres catalog that CI provides.Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)