hudi-agent commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3945053808
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -252,19 +241,7 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
HoodieFileIndex(sparkSession, metaClient, Some(tableStructSchema),
optParams,
FileStatusCache.getOrCreate(sparkSession), shouldIncludeLogFiles())
- lazy val tableState: HoodieTableState = {
- val recordMergerImpls =
optParams.get(HoodieWriteConfig.RECORD_MERGE_IMPL_CLASSES.key()).map(impls =>
ConfigUtils.split2List(impls).asScala.toList).getOrElse(List.empty)
- // Subset of the state of table's configuration as of at the time of the
query
- HoodieTableState(tablePath = basePath.toString,
- latestCommitTimestamp = queryTimestamp,
- recordKeyField = recordKeyField,
- orderingFields = orderingFields,
- usesVirtualKeys = !tableConfig.populateMetaFields(),
- metadataConfig = fileIndex.getMetadataConfig,
- recordMergeImplClasses = recordMergerImpls,
- recordMergeStrategyId = tableConfig.getRecordMergeStrategyId
- )
- }
+ protected lazy val latestCommitTimestamp: Option[String] = queryTimestamp
Review Comment:
🤖 With `tableState` gone, `orderingFields` (and I think `recordKeyField`
too) look like they no longer have any in-repo consumer — a quick grep across
the datasource modules only turns up their definitions here. Worth folding
their removal into this cleanup, or is something outside this module reading
them?
<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/HoodieBaseRelation.scala:
##########
@@ -252,19 +241,7 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
HoodieFileIndex(sparkSession, metaClient, Some(tableStructSchema),
optParams,
FileStatusCache.getOrCreate(sparkSession), shouldIncludeLogFiles())
- lazy val tableState: HoodieTableState = {
- val recordMergerImpls =
optParams.get(HoodieWriteConfig.RECORD_MERGE_IMPL_CLASSES.key()).map(impls =>
ConfigUtils.split2List(impls).asScala.toList).getOrElse(List.empty)
- // Subset of the state of table's configuration as of at the time of the
query
- HoodieTableState(tablePath = basePath.toString,
- latestCommitTimestamp = queryTimestamp,
- recordKeyField = recordKeyField,
- orderingFields = orderingFields,
- usesVirtualKeys = !tableConfig.populateMetaFields(),
- metadataConfig = fileIndex.getMetadataConfig,
Review Comment:
🤖 I think the delta may be narrower than this:
`HoodieBaseRelation.sizeInBytes` (line 291) is `fileIndex.sizeInBytes` and
isn't overridden by the MOR relations, and Spark's
`LogicalRelation.computeStats` falls back to `relation.sizeInBytes` whenever
there are no catalog stats -- so the eager listing is still forced during
planning, arguably even earlier than `composeRDD`. Do you have a case in mind
where the relation is scanned without stats ever being requested? Otherwise the
ordering change might be limited to catalog-table reads with stats present.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -100,7 +100,7 @@ class HoodieMergeOnReadRDDV2(@transient sc: SparkContext,
fileReaders: HoodieMergeOnReadBaseFileReaders,
tableSchema: HoodieTableSchema,
requiredSchema: HoodieTableSchema,
- tableState: HoodieTableState,
+ latestCommitTimestamp: Option[String],
Review Comment:
🤖 Agree the `Some`/`None` distinction is never read, but resolving to a
`String` at relation construction would regress the empty-table case:
`queryTimestamp` is `None` when the timeline has no completed instants, and
`listLatestFileSlices` returns `Seq()` there (HoodieBaseRelation:383), so the
query returns empty today instead of throwing. Resolving it where `composeRDD`
is called (splits already non-empty at that point) would get the driver-side
fail-fast without that regression.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -252,19 +241,7 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
HoodieFileIndex(sparkSession, metaClient, Some(tableStructSchema),
optParams,
FileStatusCache.getOrCreate(sparkSession), shouldIncludeLogFiles())
- lazy val tableState: HoodieTableState = {
- val recordMergerImpls =
optParams.get(HoodieWriteConfig.RECORD_MERGE_IMPL_CLASSES.key()).map(impls =>
ConfigUtils.split2List(impls).asScala.toList).getOrElse(List.empty)
- // Subset of the state of table's configuration as of at the time of the
query
- HoodieTableState(tablePath = basePath.toString,
- latestCommitTimestamp = queryTimestamp,
- recordKeyField = recordKeyField,
- orderingFields = orderingFields,
- usesVirtualKeys = !tableConfig.populateMetaFields(),
- metadataConfig = fileIndex.getMetadataConfig,
- recordMergeImplClasses = recordMergerImpls,
- recordMergeStrategyId = tableConfig.getRecordMergeStrategyId
- )
- }
+ protected lazy val latestCommitTimestamp: Option[String] = queryTimestamp
Review Comment:
🤖 One caveat on the `def queryTimestamp` suggestion: the freeze isn't
introduced by this PR -- `lazy val tableState` on master already captured
`queryTimestamp` once at first access, so moving to a `def` recomputed at each
`composeRDD` call would itself be a behavior change relative to master, in a PR
that claims none. If the recompute is the behavior we want, it'd probably be
worth calling out explicitly rather than landing it as a side effect of the
narrowing.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -75,16 +74,6 @@ trait HoodieFileSplit {}
case class HoodieTableSchema(structTypeSchema: StructType, schema:
HoodieSchema, internalSchema: Option[InternalSchema] = None)
-case class HoodieTableState(tablePath: String,
- latestCommitTimestamp: Option[String],
- recordKeyField: String,
- orderingFields: List[String],
- usesVirtualKeys: Boolean,
- metadataConfig: HoodieMetadataConfig,
- recordMergeImplClasses: List[String],
- recordMergeStrategyId: String)
Review Comment:
🤖 Worth noting the precedent cuts a bit the other way: this case class has
been reshaped in place several times without a deprecation cycle --
`preCombineField` -> `orderingFields` in #13526, the merge-mode fields in
#11943, and a field dropped again in #19458 -- so in practice it has been
treated as internal despite the missing `private[hudi]`. That said, I agree the
"no public API change" line in the description overstates it; softening that
(or noting the removal in release notes) seems like the cheaper fix here than
keeping a deprecated shell.
--
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]
