fresh-borzoni commented on PR #586:
URL: https://github.com/apache/fluss-rust/pull/586#issuecomment-4612303465
## Review — looks solid, a few minor notes
Verified the marshaling path end-to-end against the core crate and rustler
behavior. The wrapper convention from #583 (`get_server_nodes` /
`Fluss.ServerNode`) is followed faithfully:
- Core accessors line up with the `from_core` builders:
`DatabaseInfo::{database_name → &str, database_descriptor →
&DatabaseDescriptor, created_time/modified_time → i64}` and
`DatabaseDescriptor::{comment → Option<&str>, custom_properties →
&HashMap<String,String>}` all exist with matching signatures.
- `FlussAdmin::get_database_info` returns `Result<DatabaseInfo>`, the NIF is
auto-registered by `rustler::init!` (no explicit list to update),
`NifStruct`/`Encoder` derive cleanly through the nested
`NifDatabaseDescriptor`, and `Option<String>`/`HashMap` encode as `nil`/string
and a string-keyed map respectively — consistent with the `String.t() | nil`
and `%{optional(String.t()) => String.t()}` typespecs.
- Imports are clean (single `HashMap` import, used once); no mix.exs
`groups_for_modules` or CHANGELOG to touch.
### Suggestions (non-blocking)
1. **Strengthen the positive test to actually exercise the nested
marshaling.** It currently asserts only the struct *shape*
(`%Fluss.DatabaseDescriptor{} = info.descriptor`). Because
`Fluss.Admin.create_database/3` always passes `None` for the descriptor, the
round-tripped values are deterministic, so we can assert them and catch any
future regression in `NifDatabaseDescriptor::from_core`:
```elixir
assert info.descriptor.comment == nil
assert info.descriptor.custom_properties == %{}
```
As written, a mis-mapping of `comment`/`custom_properties` would still
pass.
2. **Minor idiom nit (optional):** `from_core` mixes
`comment().map(String::from)` with `database_name().to_string()`. Harmless,
just slightly inconsistent.
3. **Optional ordering:** `get_database_info` is wedged between
`create_database` and `drop_database` (two write ops), while the other read op
`database_exists` sits further down. Grouping the read operations would read a
touch cleaner, but not important.
Nothing blocking from my side — the change is correct and
convention-faithful.
--
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]
