linliu-code commented on code in PR #677:
URL: https://github.com/apache/hudi-rs/pull/677#discussion_r3771217389
##########
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.
Review Comment:
Done in ef0a708 — and it turned out neither fixing `get_or_default` nor
adding a new accessor was needed: `HudiConfigs::try_get` already has exactly
these semantics. Its doc says it "Returns `Err` on parse failures (e.g. `"yes"`
for a bool config) instead of silently falling back to the default." I had
hand-rolled a worse copy of a method the crate has had all along.
So the lookup is now:
```rust
match self.hudi_configs.try_get(HudiReadConfig::FileGroupReaderVersion)? {
Some(value) =>
FileGroupReaderVersion::try_from(usize::from(value)).map_err(CoreError::Config),
None => Ok(FileGroupReaderVersion::default()),
}
```
with a new `TryFrom<usize>` on the enum so the reader consumes the
already-parsed integer rather than re-parsing the string. `get_or_default`
stays as-is — leniency is the right default for most configs; it's specifically
wrong here, where silently falling back would run the *other* reader while the
caller believes they exercised the one they asked for.
`test_file_group_reader_version_unrecognised_returns_config_error` pins that.
##########
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()
Review Comment:
Fixed in ef0a708. `as_options()` clones the entire option map, so both
lookups were copying it on every read. The version lookup now goes through
`try_get`, which borrows.
The merge-mode lookup still reads a raw key — this crate has no typed config
for `hoodie.record.merge.mode` yet, and adding one belongs with the reader that
acts on it rather than with this seam. But it no longer clones: I added a
borrowing `HudiConfigs::get_raw` next to the existing `contains`, documented as
the escape hatch for keys with no typed config, pointing at `try_get` where one
exists.
The one remaining `as_options()` in this file is in the constructor, which
runs once per reader rather than once per read.
--
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]
