linliu-code commented on PR #698:
URL: https://github.com/apache/hudi-rs/pull/698#issuecomment-5531384744
Reviewed at `4bbb7d3` (rebased onto `eac9070`, mergeable). Flagging up front
that **no CI has run on this head** — `gh pr checks 698` reports nothing, so
everything below is from building locally rather than from a green/red signal.
Given the 0.5.0 code freeze it's worth getting a run authorized before this
merges.
Thanks for picking up the rebase @yihua, and thanks @wirybeaver for the
follow-up commit — `4bbb7d3` carries the bump across main's newer code cleanly,
and it fixed everything I'd found on the previous head except Lance.
## What I verified as correct
- `cargo clippy -p hudi-core --lib --no-default-features -- -D warnings` →
**exit 0** on this head.
- `storage/counting.rs` is right. `head` is no longer a trait method, and
inferring it from `options.head` inside `get_opts` matches how object_store
0.13 actually routes it — `ObjectStoreExt::head` is literally
`get_opts(location, GetOptions::new().with_head(true))`
(`object_store-0.13.2/src/lib.rs:1363`). Same for `delete` → `delete_stream`
and `copy`/`copy_if_not_exists` → `copy_opts`. The read-volume counters from
#708 keep their meaning.
- `set_max_row_group_size(n)` → `set_max_row_group_row_count(Some(n))` is
semantically identical — max *rows*, not bytes (the old getter is deprecated in
favour of exactly this field in `parquet-59.3.0/src/file/properties.rs:356`).
The 3-row-group fixture in `statistics_tests.rs` still does what it says.
- The DataFusion 55 adaptation is faithful to the new trait. `as_any`
removal is required (`ExecutionPlan: Any + Debug`, `TableProvider: Any + Debug
+ Sync + Send`), `apply_expressions` has no default body so the no-op impl is
necessary and matches DataFusion's own documented "node with no expressions"
example, and `Session::create_physical_expr` is equivalent to the old free
function plus `execution_props()`.
- Keeping the deprecated `ParquetObjectReader` behind per-use-site
`#[allow(deprecated)]` with a comment explaining the deferral is a reasonable
call. Might be worth a tracking issue so it isn't lost.
- Both dependency bumps are genuinely forced, for anyone wondering:
`arrow-pyarrow` 59.2 requires pyo3 `^0.29`, and `apache/datafusion-ballista`
main is already on `datafusion = "55"` / `arrow = "59.2.0"` / `object_store =
"0.13.2"`. (Note the latest *published* ballista, 54.1.0, is still on
`datafusion ^54` — worth saying "Ballista main" in the description so the
target is unambiguous.)
## Blocking
### 1. The `lance` feature does not compile, so `make check` and `make test`
cannot pass
Both targets pass `--all-features` (`Makefile:110`, `Makefile:129`), which
turns `lance` on. On this head:
```
$ cargo metadata --all-features --format-version 1 # resolves two of each
object_store: ['0.12.5', '0.13.2']
arrow: ['57.3.1', '59.3.0']
$ cargo check -p hudi-core --no-default-features --features lance
error[E0308]: mismatched types (x4)
error[E0308]: arguments to this function are incorrect
error[E0599]: no method named `head` found for `Arc<dyn ObjectStore>`
error: could not compile `hudi-core` (lib) due to 6 previous errors
exit 101
```
The root cause is that a Cargo feature gates whether code is *compiled*, not
which version of a shared dependency it links against — so it can't resolve
this diamond. `lance-io` 4.0.x pins `arrow ^57` + `object_store ^0.12.3`; this
PR moves `hudi-core` to arrow 59.2 + object_store 0.13.2; both land in one
graph and the identically-named types are distinct.
The constraint is real — there is no Lance release on arrow 59 (newest
`lance-io` is 11.0.0, on `arrow ^58`) — but the feature gate doesn't satisfy
it. The honest options are:
1. **Target DataFusion 54 / arrow 58 and bump `lance` to 11.0.0.** DF 54.1
needs `arrow ^58.3` + `object_store ^0.13.2`; lance-io 11.0.0 needs `arrow ^58`
+ `object_store ^0.13.2`. Fully consistent, keeps Lance, needs no feature gate
— but doesn't reach Ballista main.
2. **Drop Lance support on this branch**, stated as a regression in the
description with a tracking issue, and remove/ignore its tests and fixtures.
3. Keep the flag and exclude `lance` from `--all-features` — needs Makefile
+ CI changes and still leaves the feature unbuildable.
This is a project call rather than something to decide inside the PR.
### 2. pyo3 bumped to 0.29 without its source migration
`python/Cargo.toml` goes `0.26` → `0.29` with no changes under
`python/src/`. The bump is correct and forced by arrow 59, but the breaking
changes across 0.27–0.29 weren't applied. From the last CI run that did execute:
- `python/src/datafusion_internal.rs:43` — `Bound::downcast` is now `cast`
- `python/src/datafusion_internal.rs:45` — type annotation needed
- `:88` — `PyCapsule::new` → `PyCapsule::new_with_value` (deprecated)
- `#[pyclass]` types deriving `Clone` now need explicit `from_py_object` /
`skip_from_py_object` (deprecated)
The last two only warn today, but will break on the next pyo3 release.
## Worth fixing before merge
**3. Lance tests aren't gated on the feature they now require.**
`crates/core/tests/table_read_tests.rs:2441` (`mod lance_tables`),
`crates/core/src/schema/resolver.rs:312`, and the five Lance fixtures in
`gold_parity_tests.rs:88-92` have no `#[cfg(feature = "lance")]`. With the
feature off — the new default — `create_base_file_reader` returns
`Err(UnsupportedBaseFileFormat("lance support is not enabled"))` and these fail
at runtime. The import and match arm in `resolver.rs` were gated (`:23`,
`:186`); the test at `:312` was missed. Currently masked by finding 1, so
expect a second wave once that's resolved.
**4. `crates/hudi` doesn't forward the `lance` feature.**
`crates/datafusion/Cargo.toml` forwards it, but the umbrella crate doesn't —
and `python/Cargo.toml` and `cpp/Cargo.toml` both depend on `hudi`, not
`hudi-core`. By the rule already documented in `crates/hudi/Cargo.toml` for
`spill-rocksdb`, no consumer of `hudi` and neither binding can turn Lance on.
So even with finding 1 resolved, Lance would be unreachable from every
published artifact.
**5. `cmd.locations[0]` at `crates/datafusion/src/lib.rs:1025`.** DataFusion
55 changed `location: String` to `locations: Vec<String>`. Two issues: it
panics on an empty vector (reachable via
`CreateExternalTableBuilder::with_locations(vec![])` — DataFusion's own planner
guards this with `let Some(location) = locations.first() else` at
`datafusion-sql-55.0.0/src/statement.rs:1862`), and more importantly `CREATE
EXTERNAL TABLE t STORED AS HUDI LOCATION 'a', 'b'` now parses fine and silently
reads only `'a'`. Suggest:
```rust
let [base_uri] = cmd.locations.as_slice() else {
return Err(DataFusionError::Plan(
"Hudi tables require exactly one LOCATION".to_string(),
));
};
```
## Non-blocking
- **`HudiDataSource::options()` (`lib.rs:313`) can expose cloud
credentials.** Storage credentials may be supplied as table options, and this
returns caller input verbatim through a `pub` accessor whose stated purpose is
shipping to remote Ballista executors. The `Debug` impl correctly omits the
field — worth keeping that way. Either redact known-sensitive keys, or document
in the doc comment that credentials must reach workers out-of-band.
- **`schema/resolver.rs:193`** — with the feature off,
`Some(BaseFileFormatValue::Lance) => Ok(None)` falls through to
`Err(CoreError::CommitMetadata("...no file path found"))`, which misdiagnoses a
disabled feature as a missing path. `create_base_file_reader` gets this right
(`"lance support is not enabled"`); worth making the two agree.
- **`hudi_exec.rs:401`** — `partition_statistics` is `#[deprecated(since =
"55.0.0", note = "Use StatisticsContext::compute instead")]`. Behaviour is
preserved today because the default `statistics_from_inputs` delegates to it,
so this isn't urgent, but `statistics_from_inputs` is the forward-looking
override.
- **`crates/core/Cargo.toml:90-94`** — the comment above the Lance deps
still says "there is no opt-out" for `protoc`. This PR creates the opt-out; the
comment is now inaccurate.
- **`crates/test/Cargo.toml:43`** — `default-features = false` on
`hudi-datafusion` drops the spill tier without `hudi-test` forwarding a
replacement feature, which is the pattern the comments in the other three
manifests exist to prevent. Possibly vestigial from the pre-rebase version?
- **`base_uri()` / `options()`** are new public API with no in-repo consumer
and no test. Reasonable given the Ballista follow-up, but a doc note naming
that consumer would help.
- **No tests are added anywhere in the PR.** Understandable for a dependency
bump, but the two new accessors and the `locations` handling are new behaviour
rather than migration.
Happy to help with any of these, particularly 2 and 3 which are mechanical.
---
<sub>Reproduction: worktree at `4bbb7d3`, `cargo check -p hudi-core
--no-default-features --features lance` and `cargo clippy -p hudi-core --lib
--no-default-features -- -D warnings`. I did not build `hudi-datafusion`,
`hudi-python`, `hudi-cpp`, or `jvm-ffi`, and ran no tests — those gaps are why
a CI run matters here.</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]
