yihua commented on code in PR #767:
URL: https://github.com/apache/hudi-rs/pull/767#discussion_r4393773974
##########
cpp/src/lib.rs:
##########
@@ -33,33 +36,156 @@ mod ffi {
type ArrowArrayStream;
}
+ /// One column predicate, `field op value`. `op` is one of `=`, `!=`, `<`,
+ /// `<=`, `>`, `>=`, `IN`, `NOT IN`; the last two take a comma-separated
+ /// `value`.
+ struct HudiFilter {
+ field: String,
+ op: String,
+ value: String,
+ }
+
+ /// Options for one read. An empty `projection` reads every column;
+ /// `hudi_options` are `key=value` read configs such as
Review Comment:
non-blocking: For the eager slice reads, the only per-read key the core
reader acts on is `hoodie.read.use.read_optimized.mode`; `as.of.timestamp`,
`start/end.timestamp` and `stream.batch_size` set here are accepted and ignored
(see the doc on `FileGroupReader::read_file_slice_from_paths`), so a C++ caller
passing a time window per read gets unfiltered rows back with no error. Could
the doc here say that read-optimized mode is the one honored per read, and that
time-window options belong on `new_file_group_reader_with_options`? The PR
description's "for free" line for `batch_size` and `as.of.timestamp` probably
wants the same correction.
##########
cpp/src/lib.rs:
##########
@@ -33,33 +36,156 @@ mod ffi {
type ArrowArrayStream;
}
+ /// One column predicate, `field op value`. `op` is one of `=`, `!=`, `<`,
+ /// `<=`, `>`, `>=`, `IN`, `NOT IN`; the last two take a comma-separated
+ /// `value`.
+ struct HudiFilter {
+ field: String,
+ op: String,
+ value: String,
+ }
+
+ /// Options for one read. An empty `projection` reads every column;
+ /// `hudi_options` are `key=value` read configs such as
+ /// `hoodie.read.use.read_optimized.mode=true`.
+ struct HudiReadOptions {
+ projection: Vec<String>,
+ filters: Vec<HudiFilter>,
+ hudi_options: Vec<String>,
Review Comment:
non-blocking: Is `hudi_options` needed on the per-read struct? A C++ caller
can already get read-optimized mode by passing
`hoodie.read.use.read_optimized.mode=true` to
`new_file_group_reader_with_options`, since `resolve_read_options` carries
reader-level read configs into every read, and that is the only per-read key
the eager slice reads act on. Dropping it would keep the new surface to
projection and filters and avoid a field where most keys are silently ignored;
if a per-read toggle on a shared reader is the use case, a `bool
read_optimized` would say so more precisely.
##########
cpp/src/lib.rs:
##########
@@ -68,86 +194,95 @@ pub struct HudiFileGroupReader {
pub fn new_file_group_reader_with_options(
base_uri: &CxxString,
options: &CxxVector<CxxString>,
-) -> std::result::Result<Box<HudiFileGroupReader>, String> {
- let base_uri = base_uri
- .to_str()
- .map_err(|e| format!("Failed to convert CxxString to str: {e}"))?;
-
- let mut opt_vec = Vec::new();
- for opt in options.iter() {
- let opt_str = opt
- .to_str()
- .map_err(|e| format!("Failed to convert CxxString to str: {e}"))?;
- if let Some((key, value)) = opt_str.split_once('=') {
- opt_vec.push((key, value))
- }
- }
+) -> Result<Box<HudiFileGroupReader>, String> {
+ guard("new_file_group_reader_with_options", || {
+ let base_uri = cxx_str(base_uri)?;
+
+ let opt_vec = key_values(cxx_strs(options)?)?;
Review Comment:
non-blocking: The constructor now throwing on an option without `=` is a
behavior change to an existing entry point that the read-options feature
doesn't depend on. Would you consider splitting it into its own `fix(cpp)` PR
so it shows up on its own in the release notes? Its justification also leans on
`hudi_options` existing on the per-read struct (see the comment on line 54).
--
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]
