voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951323072
##########
hudi-common/src/main/java/org/apache/hudi/core/read/BaseHoodieTableFileIndex.java:
##########
@@ -97,7 +97,6 @@ public abstract class BaseHoodieTableFileIndex implements
AutoCloseable {
@Getter(AccessLevel.PROTECTED)
private final String[] partitionColumns;
- @Getter
protected final HoodieMetadataConfig metadataConfig;
Review Comment:
**major:** With the getter gone, nothing in the repo covers how
`metadataConfig` is actually built: the conjunction
`enable(props.getBoolean(...) && isFilesPartitionAvailable(metaClient) &&
!useLatestBaseFilesPathFilterForListing)` below. Both added conjuncts came from
real fixes -- `7b17e6fa1f7e` (HUDI-5403 / #7488, a Trino listing perf
regression) and `181af01a1a25` (#18136). The test removed here asserted a
Lombok field read, never this.
Not blocking, and the gap predates this PR. Could we parameterize
`TestLocalIndex` in `hudi-hadoop-common/.../TestBaseHoodieTableFileIndex.java`,
which already builds a real index over a real meta client, to assert the truth
table?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieBaseRelation.scala:
##########
@@ -244,28 +233,16 @@ abstract class HoodieBaseRelation(val sqlContext:
SQLContext,
/**
* NOTE: PLEASE READ THIS CAREFULLY
*
- * Even though [[HoodieFileIndex]] initializes eagerly listing all of the
files w/in the given Hudi table,
- * this variable itself is _lazy_ (and have to stay that way) which
guarantees that it's not initialized, until
+ * Even though [[HoodieFileIndex]] does eager work on construction (it opens
the metadata-table reader
+ * and reloads the active timeline, and lists every file in the table when
+ * `hoodie.datasource.read.file.index.listing.mode` is `eager`; it defaults
to `lazy`), this variable
+ * itself is _lazy_ (and have to stay that way) which guarantees that it's
not initialized, until
* it's actually accessed
*/
lazy val fileIndex: HoodieFileIndex =
HoodieFileIndex(sparkSession, metaClient, Some(tableStructSchema),
optParams,
FileStatusCache.getOrCreate(sparkSession), shouldIncludeLogFiles())
Review Comment:
**minor:** Removing `tableState` here also dropped a
`metaClient.reloadActiveTimeline()`: the `HoodieFileIndex` constructor calls
`doRefresh()`, which reloads at `BaseHoodieTableFileIndex.java:562`. On the MOR
incremental non-full-scan path `collectFileSplits` never touches `fileIndex`,
so the non-transient `metaClient` now reaches executors without that reload.
I could not construct a wrong-result case, since the instant range is fixed
before `composeRDD`. Could we note it in the Impact section next to the listing
change?
--
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]
