hudi-agent commented on code in PR #665:
URL: https://github.com/apache/hudi-rs/pull/665#discussion_r3771659553
##########
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:
🤖 For incremental reads, `read_base_file_eager` filters base rows per-row by
`_hoodie_commit_time` (`apply_commit_time_filter`), but routing base-file-only
slices through `read_via_v2` swaps that for the engine's file-level
`apply_instant_range_filter`. For a CoW/read-optimized incremental read under
`merge.engine=v2`, wouldn't a base file created inside the window still carry
forward older rows and return them, where the legacy path filters them out? The
snapshot-only equality test wouldn't catch this.
<sub><i>⚠️ AI-generated; verify before applying. React 👍/👎 to flag
quality.</i></sub>
##########
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:
🤖 nit: the new implementation never returns `Ok(None)` — it either produces
a schema or an `Err` — so `Result<Option<SchemaRef>>` quietly misleads callers
into guarding a `None` branch that can never fire. Could you simplify the
return type to `Result<arrow_schema::SchemaRef>` and update the call site
accordingly?
<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]
