mengw15 commented on code in PR #7539:
URL: https://github.com/apache/texera/pull/7539#discussion_r3778034854
##########
amber/src/test/integration/org/apache/texera/amber/storage/result/iceberg/IcebergDocumentSpec.scala:
##########
@@ -114,32 +113,33 @@ class IcebergDocumentSpec extends
VirtualDocumentSpec[Tuple] with BeforeAndAfter
val (batch1, batch2) = items.splitAt(batchSize)
// Write two separate batches to produce two committed data files.
- // This also initialises `document.catalog` (lazy val) with the real
catalog, which
- // is why we open a fresh reader document below after injecting the spy.
val writer1 = document.writer(UUID.randomUUID().toString)
writer1.open(); batch1.foreach(writer1.putOne); writer1.close()
val writer2 = document.writer(UUID.randomUUID().toString)
writer2.open(); batch2.foreach(writer2.putOne); writer2.close()
- val refreshCount = new AtomicInteger(0)
+ val metadataLoadCount = new AtomicInteger(0)
val realCatalog = IcebergCatalogInstance.getInstance()
- IcebergCatalogInstance.replaceInstance(catalogWithRefreshSpy(realCatalog,
refreshCount))
- // Open a fresh reader: its `catalog` lazy val hasn't been initialised
yet, so it
- // will pick up the spy catalog on first access inside seekToUsableFile.
+ IcebergCatalogInstance.replaceInstance(
+ catalogWithMetadataLoadSpy(realCatalog, metadataLoadCount)
+ )
+ // Open a fresh reader; it resolves its catalog per use (#7290), so every
metadata
+ // load inside seekToUsableFile goes through the spy installed above.
Review Comment:
The rename is load-bearing: the spy counted `table.refresh()`, and after
this PR the reader re-resolves the table instead of refreshing it — the
assertion would have been vacuously true (0 <= 4) and could no longer catch a
lazy-advancement regression. It now counts `loadTable`, where those metadata
loads surface.
##########
amber/src/test/integration/org/apache/texera/amber/storage/result/iceberg/IcebergDocumentSpec.scala:
##########
@@ -114,32 +113,33 @@ class IcebergDocumentSpec extends
VirtualDocumentSpec[Tuple] with BeforeAndAfter
val (batch1, batch2) = items.splitAt(batchSize)
// Write two separate batches to produce two committed data files.
- // This also initialises `document.catalog` (lazy val) with the real
catalog, which
- // is why we open a fresh reader document below after injecting the spy.
val writer1 = document.writer(UUID.randomUUID().toString)
writer1.open(); batch1.foreach(writer1.putOne); writer1.close()
val writer2 = document.writer(UUID.randomUUID().toString)
writer2.open(); batch2.foreach(writer2.putOne); writer2.close()
- val refreshCount = new AtomicInteger(0)
+ val metadataLoadCount = new AtomicInteger(0)
val realCatalog = IcebergCatalogInstance.getInstance()
- IcebergCatalogInstance.replaceInstance(catalogWithRefreshSpy(realCatalog,
refreshCount))
- // Open a fresh reader: its `catalog` lazy val hasn't been initialised
yet, so it
- // will pick up the spy catalog on first access inside seekToUsableFile.
+ IcebergCatalogInstance.replaceInstance(
+ catalogWithMetadataLoadSpy(realCatalog, metadataLoadCount)
+ )
+ // Open a fresh reader; it resolves its catalog per use (#7290), so every
metadata
+ // load inside seekToUsableFile goes through the spy installed above.
val readerDoc = getDocument
try {
val retrieved = readerDoc.get().toList
assert(
retrieved.toSet == items.toSet,
"All records from both files should be read correctly"
)
- // With lazy file advancement seekToUsableFile() (and therefore
table.refresh()) is called:
- // once on iterator creation, once when the last file is exhausted → 2
total.
- // Without the fix it would be called once per hasNext() on the last
file → O(batchSize).
+ // With lazy file advancement the table is (re-)resolved once at iterator
+ // construction plus once per seekToUsableFile — construction seek and
the
+ // final exhausted-files seek → 3 total. Without lazy advancement it
would be
+ // once per hasNext() on the last file → O(batchSize).
assert(
- refreshCount.get() <= 4,
- s"table.refresh() should be called at most 4 times (lazy advancement),
but was ${refreshCount.get()}"
+ metadataLoadCount.get() <= 4,
+ s"the table should be loaded at most 4 times (lazy advancement), but
was ${metadataLoadCount.get()}"
Review Comment:
Same rename as above. The comment also needed correcting on its own: it said
3 total, stale since the iterator's eager constructor-time load was dropped —
it is 2 now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
