codope commented on code in PR #12124:
URL: https://github.com/apache/hudi/pull/12124#discussion_r1806749930
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/SparkBaseIndexSupport.scala:
##########
@@ -77,20 +77,24 @@ abstract class SparkBaseIndexSupport(spark: SparkSession,
def invalidateCaches(): Unit
- protected def getPrunedFileNames(prunedPartitionsAndFileSlices:
Seq[(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])],
- includeLogFiles: Boolean = false):
Set[String] = {
- prunedPartitionsAndFileSlices
+ protected def getPrunedPartitionsAndFileNames(prunedPartitionsAndFileSlices:
Seq[(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])],
+ includeLogFiles: Boolean =
false): (Set[String], Set[String]) = {
+ val prunedPartitions : scala.collection.mutable.Set[String] =
scala.collection.mutable.Set.empty[String]
+ val prunedFiles = prunedPartitionsAndFileSlices
.flatMap {
case (_, fileSlices) => fileSlices
}
.flatMap { fileSlice =>
+ prunedPartitions.add(fileSlice.getPartitionPath)
Review Comment:
aren't we already getting partition path in the left side of each tuple in
`(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])`? If we can
use that then we don't need to allocate and keep adding to prunedPartitions.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/SparkBaseIndexSupport.scala:
##########
@@ -77,20 +77,24 @@ abstract class SparkBaseIndexSupport(spark: SparkSession,
def invalidateCaches(): Unit
- protected def getPrunedFileNames(prunedPartitionsAndFileSlices:
Seq[(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])],
- includeLogFiles: Boolean = false):
Set[String] = {
- prunedPartitionsAndFileSlices
+ protected def getPrunedPartitionsAndFileNames(prunedPartitionsAndFileSlices:
Seq[(Option[BaseHoodieTableFileIndex.PartitionPath], Seq[FileSlice])],
+ includeLogFiles: Boolean =
false): (Set[String], Set[String]) = {
+ val prunedPartitions : scala.collection.mutable.Set[String] =
scala.collection.mutable.Set.empty[String]
+ val prunedFiles = prunedPartitionsAndFileSlices
.flatMap {
case (_, fileSlices) => fileSlices
}
.flatMap { fileSlice =>
+ prunedPartitions.add(fileSlice.getPartitionPath)
val baseFileOption = Option(fileSlice.getBaseFile.orElse(null))
val logFiles = if (includeLogFiles) {
fileSlice.getLogFiles.iterator().asScala.map(_.getFileName).toList
} else Nil
baseFileOption.map(_.getFileName).toList ++ logFiles
}
.toSet
+
+ (prunedPartitions.toSet, prunedFiles)
Review Comment:
prunedPartitions is already a set, so why doing toSet again?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/ColumnStatsIndexSupport.scala:
##########
@@ -332,7 +331,7 @@ class ColumnStatsIndexSupport(spark: SparkSession,
colStatsDF.select(targetColumnStatsIndexColumns.map(col): _*)
}
- def loadColumnStatsIndexRecords(targetColumns: Seq[String],
shouldReadInMemory: Boolean): HoodieData[HoodieMetadataColumnStats] = {
+ def loadColumnStatsIndexRecords(targetColumns: Seq[String],
prunedPartitions: Option[Set[String]] = None, shouldReadInMemory: Boolean):
HoodieData[HoodieMetadataColumnStats] = {
Review Comment:
let's add UT for this method or the caller of this method? Just want to
ensure that partitions fetched from `getPrunedPartitionsAndFileNames` and
encodng done here are correct. In the current test in ths patch, I see
`prunedPartitions` is set as None here
https://github.com/apache/hudi/pull/12124/files#diff-d639b070298814506c692353524fecf43ca61a533f784f1b989dbc45ce67bbd5R197
--
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]
