linliu-code commented on code in PR #677:
URL: https://github.com/apache/hudi-rs/pull/677#discussion_r3770960959
##########
crates/core/src/file_group/reader.rs:
##########
@@ -221,6 +222,88 @@ impl FileGroupReader {
.await
}
+ /// Which merge implementation serves this read.
+ ///
+ /// A metadata table is always served by the legacy reader whatever the
Review Comment:
Done in 42e1fab — the rename left "legacy reader"/"legacy engine" behind in
the docs, which is exactly the confusion the rename was meant to remove. There
are now zero occurrences of "legacy" in this file; it reads "version 1"
throughout.
##########
crates/core/src/file_group/reader.rs:
##########
@@ -244,6 +327,29 @@ impl FileGroupReader {
.collect();
let base_file_only = log_file_paths.is_empty() ||
options.is_read_optimized()?;
+ if self.file_group_reader_version()? == FileGroupReaderVersion::Two {
+ match self.version_two_unsupported_reason(&options)? {
+ None => {
+ // Claiming a capability is claiming the rows are right,
and
+ // nothing in this crate can check that at runtime: a
reader
+ // that knew its answer was wrong would not be wrong. Only
a
+ // differential comparison against Hudi's own reader can,
and
+ // that is a test harness. So a capability may only be
added
+ // together with fixture coverage proving it.
+ return Err(CoreError::Unsupported(
+ "The merge-on-read engine reports itself able to serve
this read, \
+ but no engine is wired up behind the switch yet. A
capability must \
+ not be claimed here before there is fixture coverage
comparing its \
+ output against Hudi's reader"
+ .to_string(),
+ ));
+ }
+ Some(reason) => {
+ log::debug!("reading '{base_file_path}' with the legacy
engine: {reason}")
Review Comment:
Done in 42e1fab. The log line now reads `reading '<path>' with file group
reader version 1: <reason>` — worth getting right since this is the message a
caller actually sees when a read falls back.
##########
crates/core/src/file_group/reader.rs:
##########
@@ -1568,3 +1674,163 @@ mod tests {
Ok(())
}
}
+
+#[cfg(test)]
+mod reader_version_seam_tests {
+ use super::*;
+ use crate::config::util::empty_options;
+ use hudi_test::SampleTable;
+
+ async fn reader_with(
+ options: impl IntoIterator<Item = (&'static str, String)>,
+ ) -> Result<FileGroupReader> {
+ let base_url = SampleTable::V6Nonpartitioned.url_to_mor_parquet();
+ FileGroupReader::new_with_options(base_url.as_ref(), options).await
+ }
+
+ /// The merge-on-read engine is the default, and nothing changes for a
caller
+ /// who sets nothing — because every capability falls back today. Making it
+ /// the default only once it were capable would put the whole behaviour
change
+ /// in one commit; this way each capability carries its own.
Review Comment:
Done in 42e1fab. Renamed all six to `test_<function>_<scenario>_<expected>`
per crates/CLAUDE.md, e.g. `test_file_group_reader_version_unset_returns_two`,
`test_file_group_reader_version_unrecognised_returns_config_error`,
`test_read_file_slice_from_paths_default_version_matches_version_one`.
One of them also asserted the fall back on top of the resolved version,
which is what the next test is for — each now asserts only what its name claims.
##########
crates/core/src/file_group/reader.rs:
##########
@@ -244,6 +327,29 @@ impl FileGroupReader {
.collect();
let base_file_only = log_file_paths.is_empty() ||
options.is_read_optimized()?;
Review Comment:
Correct — `read_base_file_eager`, the base file reader, with no merging.
Neither file group reader is involved, and the version setting does not reach
that branch.
Added a comment at the branch in 42e1fab saying so, since it is also the
reasoning behind the fix for your other point on line 278: a read with nothing
to merge should not be refused for a merge-mode it never consults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
