geoHeil opened a new pull request, #27056:
URL: https://github.com/apache/datafusion/pull/27056
## Which issue does this PR close?
- Closes #27004.
## Rationale for this change
A process can decode protobuf plans and values that it did not encode
itself. A distributed worker gets them from a coordinator, and a service gets
them from a client. In that setting, a malformed message is input and should
fail with an error. Today four sites in `datafusion-proto-common`'s
`from_proto` panic on such input. A process built with `panic = "abort"` cannot
catch that panic, so one bad message takes the whole process down instead of
failing one request.
## What changes are included in this PR?
All changes are in `datafusion/proto-common/src/from_proto/mod.rs`:
- **Decimal `ScalarValue`s.** `vec_to_array` now returns `Result` instead of
panicking when the byte string has the wrong length. The four
`Decimal{32,64,128,256}Value` arms propagate the error. Text: `Expected 16
bytes for Decimal128Value but got 15`.
- **Nested `ScalarValue`s** (List, LargeList, FixedSizeList, ListView,
LargeListView, Struct, Map).
- `record_batch.column(0)` becomes `columns().first()`, which errors on a
zero-column batch. Text: `No column in record batch while deserializing nested
ScalarValue`.
- The `as_list` / `as_struct` / `as_map` / … downcasts become their `_opt`
forms. An array type that disagrees with the value's kind now errors. Text:
`Unexpected array type <data type> for ListValue while deserializing nested
ScalarValue`.
- **`Union` data type.** Without explicit `type_ids`,
`UnionFields::from_fields` is replaced by `UnionFields::try_from_fields`. This
errors instead of panicking when there are more than 128 children. The error
carries the same `Deserializing Union DataType` context as the existing
`type_ids` branch.
- **`CsvOptions`.** `delimiter[0]` and `quote[0]` become `first()`. An empty
field returns `CsvOptions: missing required field 'delimiter'` (or `'quote'`)
as a plan error, the same form the `Constraint` decoding uses.
Well-formed input decodes exactly as before.
## What is the testing strategy for this PR?
Four unit tests in `from_proto/mod.rs`. Each first checks that the
well-formed message still decodes, then corrupts it and asserts the error. Each
corrupted message panics on `main`.
- `decimal_value_of_wrong_length_is_an_error`: encodes each decimal width,
drops one byte, and asserts the error text.
- `nested_value_whose_batch_disagrees_with_its_kind_is_an_error`:
- Relabels an encoded list batch as each other nested kind, and an encoded
struct batch as a list. Asserts the type-mismatch error for each.
- Pairs a list batch with an empty schema, so the decoded batch has no
column, and asserts the missing-column error.
- `union_of_more_than_128_fields_without_type_ids_is_an_error`: 128 children
decode, and 129 return the error.
- `csv_options_without_delimiter_or_quote_is_an_error`: default options
round-trip. An empty `delimiter`, and separately an empty `quote`, return the
plan error.
## Are there any user-facing changes?
No API change. Malformed input that used to panic now returns an `Err` from
the same `TryFrom` impls.
AI assistance disclosure: this change was written with an AI coding
assistant (Claude Code), porting fixes carried in a downstream copy of this
crate. One assumption to call out: the error texts and their variants
(`Error::General` for `ScalarValue`, a plan error for `CsvOptions`) follow the
neighbouring code. Reviewers may prefer a different variant.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
