hudi-agent commented on code in PR #677:
URL: https://github.com/apache/hudi-rs/pull/677#discussion_r3770117340
##########
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
+ /// setting says: its base files and log blocks are HFile, which the
+ /// merge-on-read engine has no support for. That is permanent, not
+ /// transitional.
+ ///
+ /// The value is read raw rather than through `get_or_default`, which falls
+ /// back to the default when a value fails to parse. A typo in the engine
+ /// name would then silently read with the other engine, leaving a caller
+ /// convinced they had exercised it — the one outcome this switch must not
+ /// produce.
+ fn file_group_reader_version(&self) -> Result<FileGroupReaderVersion> {
+ if self.is_metadata_table() {
+ return Ok(FileGroupReaderVersion::One);
+ }
+ match self
+ .hudi_configs
+ .as_options()
+ .get(HudiReadConfig::FileGroupReaderVersion.as_ref())
+ {
+ Some(raw) =>
FileGroupReaderVersion::from_str(raw).map_err(CoreError::Config),
+ None => Ok(FileGroupReaderVersion::default()),
+ }
+ }
+
+ /// Why the merge-on-read engine cannot serve this read, if it cannot.
+ ///
+ /// This is a capability check, decided from config before any I/O — never
a
+ /// catch-all on error. A read that fails *inside* the engine propagates:
+ /// retrying it on the legacy reader would make a bug look like a success,
+ /// make results depend on which engine happened to win, and leave the
+ /// differential tests unable to see anything.
+ ///
+ /// Every reason here means the legacy reader serves the read instead, so
+ /// selecting the engine cannot turn a working read into a failing one.
Each
+ /// reason is logged, because a fallback nobody can observe is
+ /// indistinguishable from an engine that is never used.
+ fn version_two_unsupported_reason(
+ &self,
+ options: &ReadOptions,
+ ) -> Result<Option<&'static str>> {
+ // Deliberately an error rather than a fallback: falling back would use
+ // the legacy reader's own merge derivation, which drops deletes on a
+ // commit-time-ordered table. Wrong rows are worse than a refusal.
+ if let Some(mode) = self
+ .hudi_configs
+ .as_options()
+ // Read by raw key: this crate has no typed config for it yet, and
+ // adding one belongs with the reader that acts on it.
+ .get("hoodie.record.merge.mode")
+ && mode.eq_ignore_ascii_case("CUSTOM")
+ {
+ return Err(CoreError::Unsupported(
Review Comment:
🤖 Because v2 is the default, this CUSTOM check runs on every non-metadata
read, and it fires before the read-optimized/base-file-only determination. So a
CoW snapshot or a read-optimized MoR read — which never merges — would now
hard-error on any table declaring `hoodie.record.merge.mode=CUSTOM` (loaded
verbatim into configs by `imbue_table_properties`), even though those reads
returned correct data before this PR. Could the refusal move below the
`base_file_only`/`is_read_optimized` determination so it only triggers when a
merge actually happens?
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 nit: the test names in this module read as prose sentences rather than the
`test_<function>_<scenario>_<expected>` convention used elsewhere in the crate
— could you rename them along those lines? For example,
`the_default_version_is_two_and_still_falls_back` could become
`test_file_group_reader_version_default_returns_two` and
`an_unrecognised_version_is_an_error` could become
`test_file_group_reader_version_unrecognised_value_returns_config_error`.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
--
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]
