Jefffrey commented on code in PR #11020:
URL: https://github.com/apache/arrow-rs/pull/11020#discussion_r3957868568
##########
arrow-ipc/src/reader.rs:
##########
@@ -923,9 +923,18 @@ fn get_dictionary_values(
let value = value_type.as_ref().clone();
let schema = Schema::new(vec![Field::new("", value, true)]);
// Read a single column
+ // `DictionaryBatch.data` is optional in the flatbuffer grammar and
+ // required by the format, so a message that omits it panics here
+ // rather than producing an error. This mirrors the check the
footer
+ // metadata already got: report it, don't unwrap it.
Review Comment:
```suggestion
```
these LLM comments are really not necessary
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and
required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not
a
+ /// reduced one.
Review Comment:
```suggestion
```
same here
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and
required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not
a
+ /// reduced one.
+ #[test]
+ fn dictionary_batch_without_data_is_an_error_not_a_panic() {
+ let bytes: &[u8] =
include_bytes!("../test/data/dictionary_batch_without_data.arrow");
+ let err =
crate::reader::FileReader::try_new(std::io::Cursor::new(bytes), None)
Review Comment:
please move this test down; it shouldnt be above the imports
##########
arrow-ipc/test/data/dictionary_batch_without_data.arrow:
##########
Review Comment:
could we refactor the test to somehow create this data in memory as part of
the test? instead of commiting a separate file
##########
arrow-ipc/src/reader.rs:
##########
@@ -2009,6 +2018,26 @@ impl<R: Read> MessageReader<R> {
#[cfg(test)]
mod tests {
+ /// A dictionary message that omits its `data` must report an error.
+ ///
+ /// `DictionaryBatch.data` is optional in the flatbuffer grammar and
required
+ /// by the format, so a file that omits it is structurally decodable but
+ /// semantically incomplete. `get_dictionary_values` used to open it with
+ /// `unwrap`, which turned a malformed file into a panic.
+ ///
+ /// The fixture is the original 2169-byte input from a fuzzing corpus, not
a
+ /// reduced one.
+ #[test]
+ fn dictionary_batch_without_data_is_an_error_not_a_panic() {
Review Comment:
```suggestion
fn test_invalid_dictionary_batch_without_data() {
```
--
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]
