kosiew commented on code in PR #24726:
URL: https://github.com/apache/datafusion/pull/24726#discussion_r3903920860
##########
datafusion/proto-common/src/from_proto/mod.rs:
##########
Review Comment:
Could we propagate the `global` conversion error here instead of unwrapping
it?
`ParquetOptions::try_from` can now return an error when a protobuf `u64`
value does not fit into `usize`. For example, a `TableParquetOptions`
containing `global: Some(ParquetOptions { data_pagesize_limit: u64::MAX, .. })`
would currently panic at the inner `Result::unwrap()` instead of returning that
conversion error.
The outer `Option::unwrap()` can also panic when `global` is absent.
I think this can follow the same pattern used in `proto-models`:
```rust
.map(ParquetOptions::try_from)
.transpose()?
.unwrap_or_default()
```
Could we also add a regression test through `TableParquetOptions` that
covers an oversized nested Parquet option, and ideally the missing `global`
case as well?
##########
datafusion/proto/tests/cases/public_conversions.rs:
##########
@@ -116,12 +123,12 @@ fn common_type_conversions_are_std_traits() {
#[test]
fn file_format_option_conversions_are_std_traits() {
Review Comment:
Would it be worth adding compile-time guards for the owned and borrowed
`Constraint` and `Constraints` conversions here as well?
These conversions are part of the public API changes in this PR, so
asserting `Error = DataFusionError` would help prevent a future refactor or
extraction from accidentally dropping the fallible conversion contract.
The protobuf constraint types would need to come from
`datafusion_proto_common::protobuf_common`, since `datafusion_proto::protobuf`
does not re-export them.
This is non-blocking, but it would make the coverage of the public
conversion changes more complete.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
