hudi-agent commented on code in PR #19912:
URL: https://github.com/apache/hudi/pull/19912#discussion_r3991417406
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieFileIndex.scala:
##########
@@ -466,10 +466,11 @@ case class HoodieFileIndex(spark: SparkSession,
override def sizeInBytes: Long = {
val size = getTotalCachedFilesSize
if (size == 0 && !enableHoodieExtension) {
- // Avoid always broadcast the hudi table if not enable HoodieExtension
logWarning("Note: Please add
'org.apache.spark.sql.hudi.HoodieSparkSessionExtension' to the Spark SQL
configuration property " +
"'spark.sql.extensions'.\n Multiple extensions can be set using a
comma-separated list.")
Long.MaxValue
+ } else if (size == 0) {
+ Long.MaxValue
Review Comment:
🤖 With the extension enabled, `size == 0` here is no longer "cache not
populated" — the pruning rule has already called `filterFileSlices`, so 0
genuinely means the pruned result is empty (e.g. `WHERE partition =
'nonexistent'` or an empty table). Reporting Long.MaxValue for those now steers
Spark away from broadcasting a relation that is actually empty, which is a
plan-quality regression for snapshot reads unrelated to the incremental bug.
Since `HoodieIncrementalFileIndex` already overrides `sizeInBytes`, could this
safety net be dropped, or scoped so it doesn't affect the base snapshot path?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieIncrementalFileIndex.scala:
##########
@@ -71,6 +71,20 @@ class HoodieIncrementalFileIndex(override val spark:
SparkSession,
}).map(fileStatus => fileStatus.getPath.toString).toArray
}
+ private val reportMaxFileSize: Boolean = options.getOrElse(
Review Comment:
🤖 nit: parsing the option with `.getOrElse(...).toBoolean` will throw a
`NumberFormatException`-style error on a malformed value rather than falling
back to the default; consider using
`options.get(key).map(_.toBoolean).getOrElse(default)` or an existing
typed-options helper if one exists in this class already.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieFileIndex.scala:
##########
@@ -466,10 +466,11 @@ case class HoodieFileIndex(spark: SparkSession,
override def sizeInBytes: Long = {
val size = getTotalCachedFilesSize
if (size == 0 && !enableHoodieExtension) {
Review Comment:
🤖 nit: since both branches now return `Long.MaxValue`, could these be
combined into a single `if (size == 0)` with the warning logged only when
`!enableHoodieExtension`? As written it reads like the two cases have different
outcomes when they don't.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadIncrementalRelationV2.scala:
##########
@@ -150,6 +151,10 @@ case class MergeOnReadIncrementalRelationV2(override val
sqlContext: SQLContext,
}
}
+ override def getIncrementalFilesSize: Long = {
+ affectedFilesInCommits.asScala.map(_.getLength).sum
Review Comment:
🤖 When `fullTableScan` is true (start/end instant archived or an affected
file was deleted, with fallback enabled), `listFileSplits` scans the whole
table but this still sums only `affectedFilesInCommits`, so the reported size
can be a large underestimate — the exact condition that triggers the broadcast
timeouts this PR is fixing. Would it make sense to return Long.MaxValue (or the
full cached size) when `fullTableScan` holds? Same applies to the V1
implementation.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
