Re: [PR] fix(proto): check integer conversions in common options and constraints [datafusion]

["kosiew (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org