linliu-code commented on code in PR #665:
URL: https://github.com/apache/hudi-rs/pull/665#discussion_r3779920826
##########
crates/core/src/file_group/reader.rs:
##########
@@ -244,7 +329,17 @@ impl FileGroupReader {
.collect();
let base_file_only = log_file_paths.is_empty() ||
options.is_read_optimized()?;
- let merged = if base_file_only {
+ // Read-optimized means the log files are not read at all, so hand the
+ // engine none rather than letting it merge them. A slice with no log
+ // files reduces to a base file read either way.
+ let merged = if self.merge_engine()? == MergeEngineValue::V2 {
Review Comment:
This was a real gap when you raised it, and a later commit in the stack
closed it: `read_via_v2` now ends with
`apply_commit_time_filter(&self.hudi_configs, merged)`, so the per-row
`_hoodie_commit_time` mask is applied to version 2's output too — after the
merge rather than before, because a row's commit time is whichever record won.
You were also right that the snapshot-only equality test wouldn't catch it.
I mutation-tested the guard: deleting that call is caught by
`incremental_admits_a_commit_by_completion_time_not_requested_time` and by
`version_two_drops_a_base_record_from_outside_the_window`, which reads a
compacted base file where `d` is a pre-window row that no log record replaces.
Separately, the test that was *supposed* to guard this at the file-group
level turned out to be vacuous — see my reply on #671, where it's been
rewritten against a fixture that can actually fail.
##########
crates/core/src/file_group/reader.rs:
##########
@@ -199,6 +199,91 @@ impl FileGroupReader {
))
}
+ /// Which merge implementation should serve this read.
+ ///
+ /// Defaults to the one that has always served it. A metadata table always
+ /// uses that one regardless of the setting: its base files and log blocks
+ /// are HFile, which the merge-on-read reader cannot read at all.
+ fn merge_engine(&self) -> Result<MergeEngineValue> {
+ if self.is_metadata_table() {
+ return Ok(MergeEngineValue::Legacy);
+ }
+ // Read the raw value rather than going 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, which is
+ // the one outcome this switch must not produce.
+ match self
+ .hudi_configs
+ .as_options()
+ .get(HudiReadConfig::MergeEngine.as_ref())
+ {
+ Some(raw) =>
MergeEngineValue::from_str(raw).map_err(CoreError::Config),
+ None => Ok(MergeEngineValue::default()),
+ }
+ }
+
+ /// The schema the merge-on-read reader needs up front, taken from the base
+ /// file itself.
+ ///
+ /// This is what the existing path effectively reads with, so the two
engines
+ /// start from the same types. It is also what the data actually has: under
+ /// schema evolution `hoodie.table.create.schema` records the table as it
was
+ /// created, and the engine evolves each batch to the required schema
anyway.
+ ///
+ /// Reading the footer costs one request. The engine reads it again when it
+ /// opens the file; collapsing the two is worth doing but is not this
change.
+ async fn resolved_data_schema(
+ &self,
+ base_file_path: &str,
Review Comment:
Fixed. `resolved_data_schema` now returns `Result<arrow_schema::SchemaRef>`
— you're right that it either produced a schema or an `Err`, so the `Option`
only invited callers to guard a branch that could never fire. Both call sites
(`read_via_v2` and `stream_via_v2`) are 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
