You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "ryux1 (via GitHub)" <gi...@apache.org> on 2026/09/06 04:39:48 UTC
[PR] refactor(spark): collapse HoodieTableState [hudi]
ryux1 opened a new pull request, #19849:
URL: https://github.com/apache/hudi/pull/19849
### Describe the issue this Pull Request addresses
Closes #19465.
`HoodieTableState` retained eight fields after the FileGroupReader migration, but `HoodieMergeOnReadRDDV2` only read `latestCommitTimestamp`. Because the case class was an RDD constructor field, every MOR task serialized the remaining table path, merge configuration, and metadata configuration without consuming them.
### Summary and Changelog
- Remove `HoodieTableState` and its construction from `HoodieBaseRelation`.
- Retain the query's latest commit timestamp directly on the relation.
- Pass only that `Option[String]` to `HoodieMergeOnReadRDDV2` from the snapshot and incremental relation variants.
### Impact
No public API or behavior change. The MOR read path serializes only the timestamp it uses instead of an otherwise write-only table-state object.
### Risk Level
Low. This is a mechanical narrowing of an internal constructor parameter. All references to `HoodieTableState`, the old named argument, and its field access were removed. `git diff --check` passes.
The local environment does not contain Java or Maven, so the Scala module could not be compiled locally; hosted CI is required for compilation and test coverage.
### Documentation Update
None; this is an internal implementation cleanup with no user-facing change.
### Contributor's checklist
- [ ] Read through [contributor's guide](https://hudi.apache.org/contribute/how-to-contribute)
- [x] Enough context is provided in the sections above
- [x] Adequate tests were added if applicable (no behavior change; existing MOR coverage exercises the constructor paths)
Developed with assistance from OpenAI Codex.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951110003
##########
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:
Pushed in `d06ca59`: `@Getter(AccessLevel.PROTECTED)` on `metadataConfig`, matching `partitionColumns` and `queryPaths` beside it, and `testGetMetadataConfigReturnsFieldValue` removed along with its two now-unused imports. Re-checked the whole repo before narrowing it; the remaining `getMetadataConfig()` hits are `HoodieWriteConfig` and `HoodieMetadataValidationContext`, unrelated classes.
Leaving the thread open for one last bit: this makes a second source- and binary-compatibility change, so could the Impact section list it next to the `HoodieTableState` removal? Happy for you to resolve once the body is updated.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951108177
##########
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:
Pushed in `d06ca59`. `targetInstantTime` now lives on `BaseMergeOnReadSnapshotRelation`, the common parent of the three MOR relations, so `BaseFileOnlyRelation` and the bootstrap relation no longer inherit it.
`queryTimestamp` widens to `protected` to allow the move, which adds no member since it was already on the class. The value is still a `lazy val`, so the capture-once timing you kept is unchanged; there is a scaladoc note on it saying why it is not a `def`.
Since you mentioned you have no Java or Maven locally: `mvn compile -Dspark3.5` is green on JDK 17 across the 14 reactor modules with checkstyle and scalastyle, and `BaseHoodieTableFileIndexTest` passes.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951381224
##########
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:
Addressed in `fde0f7d8`. `TestLocalIndex` now takes the RO-path-filter flag, and an 8-row truth table asserts `metadataConfig.isEnabled()` through the real constructor rather than a planted field.
Since the complaint was that the removed test discriminated nothing, I mutation-checked this one instead of trusting a green run: dropping each of the three conjuncts from the production expression in turn makes exactly one row fail (the RO-filter conjunct, `isFilesPartitionAvailable`, and the `hoodie.metadata.enable` term).
##########
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:
Documented in the Impact section: on the MOR incremental non-full-scan path the `metaClient` serialized into the RDD is no longer preceded by the file index's `reloadActiveTimeline()`. The instant range each task reads is fixed before `composeRDD`, so it is a timing change rather than a visibility one.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951046054
##########
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:
Reopening for visibility rather than to block.
The `HoodieTableState` thread landed on accepting a documented source- and binary-compatibility break, so scoping out this much smaller narrowing reads a little inconsistent. And as it stands `testGetMetadataConfigReturnsFieldValue` now reflectively sets a field and asserts the getter hands it back, with no production caller behind either.
Could we fold the `@Getter(AccessLevel.PROTECTED)` change plus that test's removal into this PR? If it really is separate, could the PR body name the follow-up so it does not get lost?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573180661
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951110003
##########
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:
Pushed in `d06ca59`: `@Getter(AccessLevel.PROTECTED)` on `metadataConfig`, matching `partitionColumns` and `queryPaths` beside it, and `testGetMetadataConfigReturnsFieldValue` removed along with its two now-unused imports. Re-checked the whole repo before narrowing it; the remaining `getMetadataConfig()` hits are `HoodieWriteConfig` and `HoodieMetadataValidationContext`, unrelated classes.
This makes a second source- and binary-compatibility change, so rather than leave it to you I have added it to the Impact section next to the `HoodieTableState` removal. Please reword it if you would put it differently.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573368172
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
}, {
"hash" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879",
"triggerID" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877)
* 6d2fab26948ee6a5a9b26379d48a62c1e536454f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573478724
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
}, {
"hash" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879",
"triggerID" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"triggerType" : "PUSH"
}, {
"hash" : "fde0f7d8bbf609df2ba41021e0c964d36e2554d1",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16880",
"triggerID" : "fde0f7d8bbf609df2ba41021e0c964d36e2554d1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877)
* 6d2fab26948ee6a5a9b26379d48a62c1e536454f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879)
* fde0f7d8bbf609df2ba41021e0c964d36e2554d1 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16880)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951324144
##########
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:
**nit:** Revisiting this: the empty-timeline case we reasoned about is unreachable through these relations. `DefaultSource.scala:332` short-circuits a table with zero completed instants to `EmptyRelation` before any MOR relation is built (`f7a77961e365`, HUDI-1850).
Keeping `Option[String]` is still right, just because it is the type `tableState.latestCommitTimestamp` had. No change needed -- flagging so the rationale on this thread does not get quoted later.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5562750131
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573120726
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951041792
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadIncrementalRelationV2.scala:
##########
@@ -271,4 +271,3 @@ trait HoodieIncrementalRelationV2Trait extends HoodieBaseRelation {
optParams.getOrElse(DataSourceReadOptions.INCR_PATH_GLOB.key, DataSourceReadOptions.INCR_PATH_GLOB.defaultValue)
}
-
Review Comment:
Reopening: `32e71c0` does not touch this hunk. `git diff a7deb61f7426..32e71c05012a -- MergeOnReadIncrementalRelationV2.scala` still ends with
```
@@ -271,4 +271,3 @@ trait HoodieIncrementalRelationV2Trait extends HoodieBaseRelation {
}
-
```
and the file at head ends `}\n` where master ends `}\n\n`. Could we push the restore?
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951105703
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadIncrementalRelationV2.scala:
##########
@@ -271,4 +271,3 @@ trait HoodieIncrementalRelationV2Trait extends HoodieBaseRelation {
optParams.getOrElse(DataSourceReadOptions.INCR_PATH_GLOB.key, DataSourceReadOptions.INCR_PATH_GLOB.defaultValue)
}
-
Review Comment:
Pushed the restore in `d06ca59`. The file ends `}\n\n` again, so the diff against master no longer touches this hunk.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous merged PR #19849:
URL: https://github.com/apache/hudi/pull/19849
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5557566745
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f670e2da8f6ec75344e26f19d16c8ad35c8ae879 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5559756434
## [Codecov](https://app.codecov.io/gh/apache/hudi/pull/19849?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 78.33%. Comparing base ([`a7deb61`](https://app.codecov.io/gh/apache/hudi/commit/a7deb61f74263d5fa4d1b71fb32b17d70263ada9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f670e2d`](https://app.codecov.io/gh/apache/hudi/commit/f670e2da8f6ec75344e26f19d16c8ad35c8ae879?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #19849 +/- ##
============================================
- Coverage 78.33% 78.33% -0.01%
+ Complexity 33936 33933 -3
============================================
Files 2543 2543
Lines 141950 141925 -25
Branches 17212 17210 -2
============================================
- Hits 111200 111178 -22
+ Misses 23051 23049 -2
+ Partials 7699 7698 -1
```
| [Components](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hudi-common](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.73% <ø> (ø)` | |
| [hudi-client](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.21% <ø> (ø)` | |
| [hudi-flink](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.51% <ø> (ø)` | |
| [hudi-spark-datasource](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `73.22% <ø> (-0.02%)` | :arrow_down: |
| [hudi-utilities](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `74.50% <ø> (ø)` | |
| [hudi-cli](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `15.13% <ø> (ø)` | |
| [hudi-hadoop](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.79% <ø> (ø)` | |
| [hudi-sync](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `75.92% <ø> (ø)` | |
| [hudi-io](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `79.93% <ø> (ø)` | |
| [hudi-timeline-service](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `83.44% <ø> (ø)` | |
| [hudi-cloud](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `65.81% <ø> (ø)` | |
| [hudi-kafka-connect](https://app.codecov.io/gh/apache/hudi/pull/19849/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `53.96% <ø> (ø)` | |
| [Flag](https://app.codecov.io/gh/apache/hudi/pull/19849/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hadoop-mr-java-client](https://app.codecov.io/gh/apache/hudi/pull/19849/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `44.09% <ø> (-0.02%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi/pull/19849?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ain/scala/org/apache/hudi/HoodieBaseRelation.scala](https://app.codecov.io/gh/apache/hudi/pull/19849?src=pr&el=tree&filepath=hudi-spark-datasource%2Fhudi-spark-common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fhudi%2FHoodieBaseRelation.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1zcGFyay1kYXRhc291cmNlL2h1ZGktc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvaHVkaS9Ib29kaWVCYXNlUmVsYXRpb24uc2NhbGE=) | `79.06% <ø> (-0.56%)` | :arrow_down: |
| [...scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala](https://app.codecov.io/gh/apache/hudi/pull/19849?src=pr&el=tree&filepath=hudi-spark-datasource%2Fhudi-spark-common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fhudi%2FHoodieMergeOnReadRDDV2.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1zcGFyay1kYXRhc291cmNlL2h1ZGktc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvaHVkaS9Ib29kaWVNZXJnZU9uUmVhZFJERFYyLnNjYWxh) | `79.64% <ø> (+0.10%)` | :arrow_up: |
| [...apache/hudi/MergeOnReadIncrementalRelationV1.scala](https://app.codecov.io/gh/apache/hudi/pull/19849?src=pr&el=tree&filepath=hudi-spark-datasource%2Fhudi-spark-common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fhudi%2FMergeOnReadIncrementalRelationV1.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1zcGFyay1kYXRhc291cmNlL2h1ZGktc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvaHVkaS9NZXJnZU9uUmVhZEluY3JlbWVudGFsUmVsYXRpb25WMS5zY2FsYQ==) | `79.81% <ø> (-0.19%)` | :arrow_down: |
| [...apache/hudi/MergeOnReadIncrementalRelationV2.scala](https://app.codecov.io/gh/apache/hudi/pull/19849?src=pr&el=tree&filepath=hudi-spark-datasource%2Fhudi-spark-common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fhudi%2FMergeOnReadIncrementalRelationV2.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1zcGFyay1kYXRhc291cmNlL2h1ZGktc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvaHVkaS9NZXJnZU9uUmVhZEluY3JlbWVudGFsUmVsYXRpb25WMi5zY2FsYQ==) | `86.27% <ø> (-0.14%)` | :arrow_down: |
| [.../org/apache/hudi/MergeOnReadSnapshotRelation.scala](https://app.codecov.io/gh/apache/hudi/pull/19849?src=pr&el=tree&filepath=hudi-spark-datasource%2Fhudi-spark-common%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Fhudi%2FMergeOnReadSnapshotRelation.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-aHVkaS1zcGFyay1kYXRhc291cmNlL2h1ZGktc3BhcmstY29tbW9uL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUvaHVkaS9NZXJnZU9uUmVhZFNuYXBzaG90UmVsYXRpb24uc2NhbGE=) | `89.36% <ø> (-0.23%)` | :arrow_down: |
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5562154674
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f670e2da8f6ec75344e26f19d16c8ad35c8ae879 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847)
* 32e71c05012af50d4df82278ee842358d907333c UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "ryux1 (via GitHub)" <gi...@apache.org>.
ryux1 commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3945240667
##########
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:
Updated the Impact and Risk sections to state explicitly that removing this publicly scoped type is a source- and binary-compatibility change for out-of-tree consumers. I chose the documented-breakage option rather than retaining a deprecated shell; the changelog now calls out the removal.
##########
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:
Both fields remain live. `recordKeyField` and `orderingFields` feed `mandatoryFieldsForMerging` in `MergeOnReadSnapshotRelation`, and `orderingFields` also feeds `mandatoryFields` in both incremental relation variants, which is then consumed by projection and schema pruning. I am leaving them in place.
##########
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:
Kept the frozen lazy value to preserve the behavior of `master`: the old lazy `tableState` captured `queryTimestamp` once, while widening the `def` would recompute it on later `composeRDD` calls. Commit `32e71c0` renames the captured value to `targetInstantTime` so the distinction is explicit without changing the timing.
##########
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:
Confirmed that the reflective test is the only remaining in-repo caller. I am leaving the getter unchanged here because reducing its visibility would introduce another public API break unrelated to the RDD serialization change. That cleanup is better isolated in a follow-up.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -89,7 +89,7 @@ private[hudi] case class HoodieMergeOnReadBaseFileReaders(fullSchemaReader: Base
* @param fileReaders suite of base file readers
* @param tableSchema table's full schema
* @param requiredSchema expected (potentially) projected schema
- * @param tableState table's state
+ * @param latestCommitTimestamp latest completed commit timestamp for the query
Review Comment:
Fixed in `32e71c0`: the parameter is now `targetInstantTime`, documented as the as-of instant or the last instant in the query timeline.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -89,7 +89,7 @@ private[hudi] case class HoodieMergeOnReadBaseFileReaders(fullSchemaReader: Base
* @param fileReaders suite of base file readers
* @param tableSchema table's full schema
* @param requiredSchema expected (potentially) projected schema
- * @param tableState table's state
+ * @param latestCommitTimestamp latest completed commit timestamp for the query
Review Comment:
Fixed in `32e71c0`: the descriptions are aligned and `sqlConf`, `optionalFilters`, `metaClient`, and `options` are now documented.
##########
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:
Keeping `Option[String]` preserves the existing empty-table path: `queryTimestamp` can be `None`, while `listLatestFileSlices` returns no splits. Resolving a mandatory `String` at relation construction would turn that case into an error. The renamed parameter documents what the optional value represents.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadIncrementalRelationV2.scala:
##########
@@ -271,4 +271,3 @@ trait HoodieIncrementalRelationV2Trait extends HoodieBaseRelation {
optParams.getOrElse(DataSourceReadOptions.INCR_PATH_GLOB.key, DataSourceReadOptions.INCR_PATH_GLOB.defaultValue)
}
-
Review Comment:
Restored the trailing blank line in `32e71c0`.
##########
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:
Good catch. The Impact section now calls out that removing `tableState` stops forcing `fileIndex` during RDD construction and may defer or avoid the listing when catalog statistics satisfy planning; paths that consult `sizeInBytes` still force it. I did not add a test because whether Spark requests relation statistics is planner/catalog behavior rather than a stable invariant of this refactor.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-agent (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573309006
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
}, {
"hash" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877)
* 6d2fab26948ee6a5a9b26379d48a62c1e536454f UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3944193962
##########
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:
`HoodieTableState` is plain `public` and ships in hudi-spark-bundle, so any out-of-repo relation subclass, custom datasource or tool that constructs one or reads `relation.tableState` breaks at compile time against the new source, and with `NoClassDefFoundError`/`NoSuchMethodError` against the new bundle. The PR body says "No public API or behavior change".
Its file-neighbour at `HoodieMergeOnReadRDDV2.scala:80` is `private[hudi] case class HoodieMergeOnReadBaseFileReaders`, so the codebase does scope internal case classes deliberately; this one never was.
Could we either keep the case class for one release marked deprecated, or drop the "no public API change" claim from the description and flag the removal in the release notes?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -89,7 +89,7 @@ private[hudi] case class HoodieMergeOnReadBaseFileReaders(fullSchemaReader: Base
* @param fileReaders suite of base file readers
* @param tableSchema table's full schema
* @param requiredSchema expected (potentially) projected schema
- * @param tableState table's state
+ * @param latestCommitTimestamp latest completed commit timestamp for the query
Review Comment:
The value passed here is `specifiedQueryTimestamp.orElse(timeline.lastInstant())`, which is not the latest completed commit at two of the three call sites:
- under time travel (`as.of.instant` / `hoodie.datasource.read.begin.instanttime`) it is the user's as-of instant;
- `MergeOnReadIncrementalRelationV1` overrides `timeline` to `findInstantsInRange(startTimestamp, endTimestamp)` plus pending compaction, and V2 to `queryContext.getActiveTimeline`, so it is the last instant of the queried window.
Could we word it as the query's target instant -- the as-of instant, or the last instant of the queried timeline -- instead? As written, a reader will assume the table's newest commit is what reaches `HoodieFileGroupReader.withLatestCommitTime` and mis-reason about merge and log-block visibility.
##########
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:
Separately: dropping this call leaves `BaseHoodieTableFileIndex#getMetadataConfig` with no in-repo production caller. The only remaining reference is `BaseHoodieTableFileIndexTest#testGetMetadataConfigReturnsFieldValue`, which reflectively sets the field and then asserts the getter hands it back.
Its neighbours in that class are already scoped -- `@Getter(AccessLevel.PROTECTED)` on `partitionColumns` and `queryPaths`. Could we drop `metadataConfig`'s `@Getter` to `AccessLevel.PROTECTED` and retire that test with it?
##########
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:
This call was the only thing forcing the `lazy val fileIndex` (line 240), whose own scaladoc warns it "initializes eagerly listing all of the files w/in the given Hudi table".
For `MergeOnReadIncrementalRelationV1`/`V2` on the non-`fullTableScan` branch, `collectFileSplits` builds its own `HoodieTableFileSystemView` from `affectedFilesInCommits` and never touches `fileIndex` -- the only `fileIndex` hits in those two files are V1:133 and V2:127, inside `listFileSplits`, a different entry point. So an incremental MOR query with a start/end range used to perform a full table listing at `composeRDD` time and now does not.
That reads like a win, but it is a real behavior delta against the description, and it changes when the listing and any listing-time failure surface. Could we call it out in the PR body and add an incremental-MOR test that pins the new behavior?
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/HoodieMergeOnReadRDDV2.scala:
##########
@@ -89,7 +89,7 @@ private[hudi] case class HoodieMergeOnReadBaseFileReaders(fullSchemaReader: Base
* @param fileReaders suite of base file readers
* @param tableSchema table's full schema
* @param requiredSchema expected (potentially) projected schema
- * @param tableState table's state
+ * @param latestCommitTimestamp latest completed commit timestamp for the query
Review Comment:
Nit: every other entry aligns its description at a fixed column and this one uses a single space, so the block no longer reads as a table.
While we are in here, the block is also missing `sqlConf`, `optionalFilters`, `metaClient` and `options`, all present in the constructor before this PR. Could we add them?
##########
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:
All three uses inside `compute()` are `latestCommitTimestamp.orNull` (lines 211, 229, 242), so the `Some`/`None` distinction is never read. Both builders then do `ValidationUtils.checkArgument(latestCommitTime != null, "Latest commit time is required")` (`HoodieFileGroupReader.java:121`, `HoodieLsmFileGroupReader.java:108`), so a `None` fails per-task on the executor rather than on the driver.
Since the PR is deliberately picking this parameter's type, could it be `String`, resolved once on the driver? That removes the wrapper and turns an executor-side `IllegalArgumentException` into a fail-fast at relation construction.
##########
hudi-spark-datasource/hudi-spark-common/src/main/scala/org/apache/hudi/MergeOnReadIncrementalRelationV2.scala:
##########
@@ -271,4 +271,3 @@ trait HoodieIncrementalRelationV2Trait extends HoodieBaseRelation {
optParams.getOrElse(DataSourceReadOptions.INCR_PATH_GLOB.key, DataSourceReadOptions.INCR_PATH_GLOB.defaultValue)
}
-
Review Comment:
This has nothing to do with collapsing `HoodieTableState` -- could we restore the trailing blank line? It widens the conflict surface for anyone cherry-picking onto release-1.2.x, and `git diff --check`, cited in the PR body as the verification, does not catch it.
##########
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:
This is a second name for something the same class already exposes. `queryTimestamp` (line 260) is a `def` recomputed from the overridable `timeline` on every call, and `listLatestFileSlices` (line 345) uses it that way; as a `lazy val`, `latestCommitTimestamp` freezes the first value. The two disagree whenever the metaClient timeline is reloaded between `collectFileSplits` and `composeRDD` on a reused relation.
It also lands on `HoodieBaseRelation`, so `BaseFileOnlyRelation`, `HoodieBootstrapRelation` and `IncrementalRelation` all inherit a member only the three MOR relations use, which widens the base class in a PR aimed at narrowing surface.
Could we widen `private def queryTimestamp` to `protected def queryTimestamp` and pass `queryTimestamp` at the three call sites instead? That adds no member at all and keeps one accessor for the concept.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5557072609
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f670e2da8f6ec75344e26f19d16c8ad35c8ae879 Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5557038038
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f670e2da8f6ec75344e26f19d16c8ad35c8ae879 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5562186683
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* f670e2da8f6ec75344e26f19d16c8ad35c8ae879 Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847)
* 32e71c05012af50d4df82278ee842358d907333c Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "ryux1 (via GitHub)" <gi...@apache.org>.
ryux1 commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5575591728
Thanks for the careful review and for pushing the follow-up fixes, @voonhous. I really appreciated the detailed reasoning around the lazy timeline capture and the metadata-config coverage.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "voonhous (via GitHub)" <gi...@apache.org>.
voonhous commented on code in PR #19849:
URL: https://github.com/apache/hudi/pull/19849#discussion_r3951044342
##########
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:
The rename settles the naming half, and the freeze rationale is right; agreed on keeping the capture-once semantics.
The second half is still open though: `targetInstantTime` is a `protected lazy val` on `HoodieBaseRelation:244`, so `BaseFileOnlyRelation`, `HoodieBootstrapRelation` and `IncrementalRelation` all inherit a member only the three MOR relations read.
Those three (`MergeOnReadSnapshotRelation`, `MergeOnReadIncrementalRelationV1`, `MergeOnReadIncrementalRelationV2`) all extend `BaseMergeOnReadSnapshotRelation`, so could the `lazy val` move there instead? Same capture-once timing, and the base class stops growing in a PR aimed at narrowing surface.
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(spark): collapse HoodieTableState [hudi]
Posted by "hudi-bot (via GitHub)" <gi...@apache.org>.
hudi-bot commented on PR #19849:
URL: https://github.com/apache/hudi/pull/19849#issuecomment-5573425181
<!--
Meta data
{
"version" : 1,
"metaDataEntries" : [ {
"hash" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"status" : "DELETED",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16847",
"triggerID" : "f670e2da8f6ec75344e26f19d16c8ad35c8ae879",
"triggerType" : "PUSH"
}, {
"hash" : "32e71c05012af50d4df82278ee842358d907333c",
"status" : "SUCCESS",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856",
"triggerID" : "32e71c05012af50d4df82278ee842358d907333c",
"triggerType" : "PUSH"
}, {
"hash" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877",
"triggerID" : "d06ca5940b797764a0bdd55f8ad488b4bd9032ae",
"triggerType" : "PUSH"
}, {
"hash" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"status" : "PENDING",
"url" : "https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879",
"triggerID" : "6d2fab26948ee6a5a9b26379d48a62c1e536454f",
"triggerType" : "PUSH"
}, {
"hash" : "fde0f7d8bbf609df2ba41021e0c964d36e2554d1",
"status" : "UNKNOWN",
"url" : "TBD",
"triggerID" : "fde0f7d8bbf609df2ba41021e0c964d36e2554d1",
"triggerType" : "PUSH"
} ]
}-->
## CI report:
* 32e71c05012af50d4df82278ee842358d907333c Azure: [SUCCESS](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16856)
* d06ca5940b797764a0bdd55f8ad488b4bd9032ae Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16877)
* 6d2fab26948ee6a5a9b26379d48a62c1e536454f Azure: [PENDING](https://dev.azure.com/apachehudi/a1a51da7-8592-47d4-88dc-fd67bed336bb/_build/results?buildId=16879)
* fde0f7d8bbf609df2ba41021e0c964d36e2554d1 UNKNOWN
<details>
<summary>Bot commands</summary>
@hudi-bot supports the following commands:
- `@hudi-bot run azure` re-run the last Azure build
</details>
--
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: commits-unsubscribe@hudi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org