You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by "xushiyan (via GitHub)" <gi...@apache.org> on 2026/04/27 19:49:47 UTC
[PR] fix: consolidate file metadata population for snapshot and incremental queries [hudi-rs]
xushiyan opened a new pull request, #586:
URL: https://github.com/apache/hudi-rs/pull/586
## Description
Fixes #401. Replaces #579 (auto-closed during rebase).
`get_file_slices_internal()` (snapshot) and `get_file_slices_between_internal()` (incremental) previously diverged on `FileMetadata` population. After #548, the snapshot path used a sample-once `FileStatsEstimator` cached on `FileSystemView`, while the incremental path still returned file slices with `file_metadata: None`.
This PR consolidates both paths around a single mechanism:
- `FileStatsEstimator` cache moves from `FileSystemView` to `Table` (single `OnceCell` shared across all queries on the same instance).
- `Table::get_or_init_estimator` is the single gatekeeper that validates parquet eligibility for the estimator. Documented as data-table-only; HFile metadata tables short-circuit through the same check.
- Estimator is seeded from a base parquet path discovered in commit metadata (latest commit ≤ as-of for snapshot; latest ≤ end-of-window for incremental). MOR delta-only-latest is handled by walking back through prior commits.
- `file_groups_from_commit_metadata` now accepts the estimator and populates `FileMetadata` from `HoodieWriteStat::file_size_in_bytes` exactly like `file_groups_from_files_partition_records`.
- `FileSystemView` is no longer responsible for estimator lifecycle: `OnceCell`, `get_or_init_estimator`, and `find_sample_base_file_path_from_records` are removed; the parquet check inside `apply_stats_pruning_from_footers` (a separate column-stats concern) is inlined.
Net effect: incremental queries now return `FileSlice`s with `size`, `byte_size`, and `num_records` populated, matching the snapshot path. The storage-listing snapshot path also gains `byte_size` / `num_records` as a free side effect of unification.
## How are the changes test-covered
- [x] Automated tests (unit and/or integration tests)
--
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: consolidate file metadata population for snapshot and incremental queries [hudi-rs]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #586:
URL: https://github.com/apache/hudi-rs/pull/586#issuecomment-4330405064
## [Codecov](https://app.codecov.io/gh/apache/hudi-rs/pull/586?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `70.11494%` with `26 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.21%. Comparing base ([`fdfc778`](https://app.codecov.io/gh/apache/hudi-rs/commit/fdfc77892134a2824c874767f3eab488166982d9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d2ff5f6`](https://app.codecov.io/gh/apache/hudi-rs/commit/d2ff5f6eb29531740f247939b691807a38242aad?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/hudi-rs/pull/586?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [crates/core/src/file\_group/builder.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ffile_group%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL2ZpbGVfZ3JvdXAvYnVpbGRlci5ycw==) | 45.45% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/table/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL21vZC5ycw==) | 75.75% | [8 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/table/fs\_view.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Ffs_view.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL2ZzX3ZpZXcucnM=) | 66.66% | [2 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/metadata/commit.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fmetadata%2Fcommit.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL21ldGFkYXRhL2NvbW1pdC5ycw==) | 94.11% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/metadata/table/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Fmetadata%2Ftable%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL21ldGFkYXRhL3RhYmxlL21vZC5ycw==) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/table/builder.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftable%2Fbuilder.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RhYmxlL2J1aWxkZXIucnM=) | 0.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [crates/core/src/timeline/mod.rs](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&filepath=crates%2Fcore%2Fsrc%2Ftimeline%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y3JhdGVzL2NvcmUvc3JjL3RpbWVsaW5lL21vZC5ycw==) | 85.71% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/hudi-rs/pull/586?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #586 +/- ##
==========================================
+ Coverage 82.08% 82.21% +0.12%
==========================================
Files 77 77
Lines 5159 5183 +24
==========================================
+ Hits 4235 4261 +26
+ Misses 924 922 -2
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/hudi-rs/pull/586?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<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.
</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: consolidate file metadata population for snapshot and incremental queries [hudi-rs]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #586:
URL: https://github.com/apache/hudi-rs/pull/586#discussion_r3150300235
##########
crates/core/src/table/mod.rs:
##########
@@ -454,13 +539,16 @@ impl Table {
None
};
+ let estimator = self.get_or_init_estimator(timestamp).await;
Review Comment:
`get_file_slices_internal` initializes the `FileStatsEstimator` unconditionally, even when `metadata_table` is `None` (metadata table disabled or failed to open). In that case `FileSystemView` falls back to storage listing and currently does not use the estimator, so this can trigger an unnecessary Parquet footer read on every snapshot/time-travel query. Consider only calling `get_or_init_estimator` when the estimator will be consumed (e.g., when `metadata_table.is_some()`), or alternatively thread the estimator into the storage-listing path so it actually populates `byte_size`/`num_records` there.
```suggestion
let estimator = if metadata_table.is_some() {
self.get_or_init_estimator(timestamp).await
} else {
None
};
```
##########
crates/core/src/table/mod.rs:
##########
@@ -454,13 +539,16 @@ impl Table {
None
};
+ let estimator = self.get_or_init_estimator(timestamp).await;
+
self.file_system_view
.get_file_slices(
&partition_pruner,
&file_pruner,
&table_schema,
&timeline_view,
metadata_table,
+ estimator,
)
Review Comment:
PR description claims the storage-listing snapshot path also gains fully populated stats. With the current implementation, `get_file_slices_internal` always computes an estimator and passes it into `FileSystemView`, but the storage-listing path does not apply that estimator to populate `byte_size`/`num_records` (only `size` is present from `Storage::list_files`). Either update the implementation to use the estimator for listing-based snapshot reads, or adjust the PR description/tests to match the actual behavior.
##########
crates/core/src/table/fs_view.rs:
##########
@@ -206,7 +125,7 @@ impl FileSystemView {
.await?
};
Review Comment:
In `load_file_groups`, the `estimator` parameter is only used when loading from metadata table records; the storage-listing branch ignores it. As a result, snapshot/time-travel queries that fall back to listing will still return `FileMetadata` with `byte_size`/`num_records` left at 0 (and may also pay the cost of initializing the estimator upstream). If the intent is to populate estimated stats for listing-based snapshot reads as well, consider plumbing `estimator` into `FileLister` and applying it when converting `FileMetadata` -> `BaseFile` (when `size > 0`).
##########
crates/core/src/table/mod.rs:
##########
@@ -164,6 +171,84 @@ impl Table {
.await
}
+ /// Get or initialize the cached `FileStatsEstimator` for this **data table**.
+ ///
+ /// This is the single point where eligibility for footer-based stats
+ /// estimation is validated. Returns `None` when:
+ /// - The base file format is not Parquet (e.g., HFile metadata tables).
+ /// - No sample base-file path can be derived from commit metadata at or
+ /// before `sample_at_timestamp`.
+ /// - The parquet footer read fails for the sample path.
+ ///
+ /// The cache is only materialized on successful initialization. This avoids
+ /// pinning a "no sample yet" state from an early timestamp.
+ ///
+ /// Not intended for use on metadata tables. MDTs use HFile, which already
+ /// short-circuits via the format check below; this is documented for the
+ /// reader, not enforced via a separate flag.
+ pub(crate) async fn get_or_init_estimator(
+ &self,
+ sample_at_timestamp: &str,
+ ) -> Option<&FileStatsEstimator> {
+ if let Some(estimator) = self.cached_estimator.get() {
+ return Some(estimator);
+ }
+
+ // SINGLE point of parquet validation for the estimator concern.
+ if !matches!(
+ self.file_system_view.base_file_format(),
+ BaseFileFormatValue::Parquet
+ ) {
+ return None;
+ }
+
+ let path = self
+ .sample_base_file_path_at_or_before(sample_at_timestamp)
+ .await?;
+
+ self.cached_estimator
+ .get_or_try_init(|| async {
+ FileStatsEstimator::from_parquet_footer(&self.file_system_view.storage, &path).await
+ })
+ .await
+ .ok()
Review Comment:
`get_or_init_estimator` drops the initialization error via `.ok()`; when footer reads fail, callers just see `None` with no context. This makes it hard to diagnose missing stats in production. Consider logging a warning (including the sampled path and error) before returning `None`, while still keeping the cache unpopulated on failures.
```suggestion
match self
.cached_estimator
.get_or_try_init(|| async {
FileStatsEstimator::from_parquet_footer(&self.file_system_view.storage, &path).await
})
.await
{
Ok(estimator) => Some(estimator),
Err(error) => {
log::warn!(
"Failed to initialize file stats estimator from sampled parquet footer at path '{}': {}",
path,
error
);
None
}
}
```
--
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: consolidate file metadata population for snapshot and incremental queries [hudi-rs]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #586:
URL: https://github.com/apache/hudi-rs/pull/586#discussion_r3150178409
##########
crates/core/src/table/mod.rs:
##########
@@ -164,6 +171,80 @@ impl Table {
.await
}
+ /// Get or initialize the cached `FileStatsEstimator` for this **data table**.
+ ///
+ /// This is the single point where eligibility for footer-based stats
+ /// estimation is validated. Returns `None` (cached as such) when:
+ /// - The base file format is not Parquet (e.g., HFile metadata tables).
+ /// - No sample base-file path can be derived from commit metadata at or
+ /// before `sample_at_timestamp`.
+ /// - The parquet footer read fails for the sample path.
+ ///
+ /// The first call materializes the cache; subsequent calls reuse the cached
+ /// `Option<FileStatsEstimator>` regardless of the timestamp passed in.
+ ///
+ /// Not intended for use on metadata tables. MDTs use HFile, which already
+ /// short-circuits via the format check below; this is documented for the
+ /// reader, not enforced via a separate flag.
+ pub(crate) async fn get_or_init_estimator(
+ &self,
+ sample_at_timestamp: &str,
+ ) -> Option<&FileStatsEstimator> {
+ self.cached_estimator
+ .get_or_init(|| async {
+ // SINGLE point of parquet validation for the estimator concern.
+ if !matches!(
+ self.file_system_view.base_file_format(),
Review Comment:
`cached_estimator` caches `None` permanently on the first call via `OnceCell<Option<_>>`. If the first query uses an early `sample_at_timestamp` where no base file exists (or footer read temporarily fails), later queries on the same `Table` instance (with a newer timestamp where a sample exists) will never attempt initialization again, so metadata stays unpopulated unexpectedly. Consider using `OnceCell<FileStatsEstimator>` with `get_or_try_init` so the cell is only set on successful initialization (and return `None` without initializing when no sample path is available), or otherwise avoid caching the "no sample" case.
##########
crates/core/src/table/fs_view.rs:
##########
@@ -173,27 +102,17 @@ impl FileSystemView {
table_schema: &Schema,
timeline_view: &TimelineView,
files_partition_records: Option<&HashMap<String, FilesPartitionRecord>>,
+ estimator: Option<&FileStatsEstimator>,
) -> Result<()> {
- let base_file_format = self.base_file_format();
- let base_file_extension = base_file_format.as_ref();
-
- // Step 1: Initialize the file stats estimator from MDT records if available.
- // For Parquet tables, this reads ONE footer to derive compression ratio and
- // avg row size, allowing the file group builder to populate estimated metadata inline.
- if let Some(records) = files_partition_records {
- let sample_path =
- Self::find_sample_base_file_path_from_records(records, &base_file_format);
- self.get_or_init_estimator(&base_file_format, sample_path.as_deref())
- .await;
- }
+ let base_file_extension = self.base_file_format();
+ let base_file_extension = base_file_extension.as_ref();
Review Comment:
`load_file_groups` shadows `base_file_extension` with a different type (`BaseFileFormatValue` then `&str`), which makes the code harder to follow and easier to misuse. Consider renaming the first binding to `base_file_format` (or avoid shadowing) so it's clear what's passed into `file_groups_from_files_partition_records`.
```suggestion
let base_file_format = self.base_file_format();
let base_file_extension = base_file_format.as_ref();
```
##########
crates/core/src/file_group/builder.rs:
##########
@@ -66,9 +66,13 @@ impl FileGroupMerger for HashSet<FileGroup> {
///
/// * `commit_metadata` - The commit metadata JSON map
/// * `completion_time_view` - View to look up completion timestamps.
-pub fn file_groups_from_commit_metadata<V: CompletionTimeView>(
+/// * `estimator` - Optional [FileStatsEstimator] used to populate `byte_size`
+/// and `num_records` on each base file. When `None`, those fields stay at 0
+/// while `size` is still populated from `HoodieWriteStat::file_size_in_bytes`.
+pub(crate) fn file_groups_from_commit_metadata<V: CompletionTimeView>(
commit_metadata: &Map<String, Value>,
completion_time_view: &V,
+ estimator: Option<&FileStatsEstimator>,
) -> Result<HashSet<FileGroup>> {
Review Comment:
`file_groups_from_commit_metadata` was changed from `pub` to `pub(crate)`, but `file_group::builder` is still a public module (`crates/core/src/file_group/mod.rs:24`). This is a breaking change for downstream users that may call this helper. Either keep the function `pub` (and treat the new `estimator` param as part of the public API), or make the `builder` module non-public if these helpers are intended to be internal only.
##########
crates/core/src/timeline/mod.rs:
##########
@@ -280,6 +284,21 @@ impl Timeline {
resolve_data_schema_from_commit_metadata(&commit_metadata, self.storage.clone()).await
}
+ /// Get all completed commit instants whose request timestamp is ≤ `timestamp`,
+ /// sorted ascending by timestamp.
+ pub(crate) fn get_completed_commits_at_or_before(
+ &self,
+ timestamp: &str,
+ ) -> Result<Vec<Instant>> {
+ let selector = TimelineSelector::completed_actions_in_range(
+ DEFAULT_LOADING_ACTIONS,
+ self.hudi_configs.clone(),
Review Comment:
The new `get_completed_commits_at_or_before` name/docs say it returns "commit instants", but it selects `DEFAULT_LOADING_ACTIONS` (commit + deltacommit + replacecommit). This mismatch can lead to incorrect assumptions by callers. Consider renaming (e.g., `get_completed_instants_at_or_before`) or tightening the selector to only `Action::Commit`, and update the doc comment accordingly.
--
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: consolidate file metadata population for snapshot and incremental queries [hudi-rs]
Posted by "xushiyan (via GitHub)" <gi...@apache.org>.
xushiyan merged PR #586:
URL: https://github.com/apache/hudi-rs/pull/586
--
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