wombatu-kun commented on code in PR #19974:
URL: https://github.com/apache/hudi/pull/19974#discussion_r4384555457
##########
hudi-spark-datasource/hudi-spark/src/main/scala/org/apache/spark/sql/hudi/command/procedures/ShowHoodieLogFileRecordsProcedure.scala:
##########
@@ -84,26 +113,21 @@ class ShowHoodieLogFileRecordsProcedure extends
BaseProcedure with ProcedureBuil
.withDiskMapType(HoodieCommonConfig.SPILLABLE_DISK_MAP_TYPE.defaultValue)
.withBitCaskDiskMapCompressionEnabled(HoodieCommonConfig.DISK_MAP_BITCASK_COMPRESSION_ENABLED.defaultValue)
.build
- scanner.asScala.foreach(hoodieRecord => {
- val record =
hoodieRecord.getData.asInstanceOf[HoodieRecordPayload[_]].getInsertValue(schema.toAvroSchema).get()
- if (allRecords.size() < limit) {
- allRecords.add(record)
- }
+ scanner.asScala.takeWhile(_ => !enoughMatches).foreach(hoodieRecord => {
Review Comment:
`scanner.asScala` is a Scala `Iterable`, so `takeWhile` here is strict: with
or without a filter, the merge branch collects every merged record (spilled
ones included) on the heap before the first `offer`, and `enoughMatches` never
stops the scan. Iterating `scanner.iterator().asScala` instead keeps
`takeWhile` lazy, so the scan stops once `limit` matches are found.
##########
hudi-spark-datasource/hudi-spark/src/test/scala/org/apache/spark/sql/hudi/procedure/TestShowTimelineTableProcedure.scala:
##########
@@ -645,6 +645,21 @@ class TestShowTimelineTableProcedure extends
HoodieSparkSqlTestBase {
val expectedArchived = filterInstantsByTimeRange(archivedInstants,
Some(startTime), Some(endTime))
verifyResults(results, expectedActive, expectedArchived, None, "Test 12:
Start and end in archived, archived enabled")
}
+
+ // Test Case 13: a fully specified time range ignores `limit`, so a small
limit must not truncate it
Review Comment:
No test passes a `filter` to `show_timeline` (Test Case 13 passes on the
base too), so its `scanLimit` lift can be reverted with every test green, as
can the `hasFilter` source bounds in `show_bootstrap_mapping`,
`show_fs_path_detail` and `show_invalid_parquet`. Could you add a `limit => 1`
case whose filter matches only an instant past the limit, like the new
`show_cleans` test, at least for `show_timeline`?
--
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]
