fix: better error handling for negative size of FixedSizeBinary

[theirix]

Which issue does this PR close?

• Closes DataType parser permits negative FixedSizeBinary size #10033.

Rationale for this change

Related to apache/datafusion#22297, where using FixedSizeBinary(-N) caused failures. Actually, it will still panic, but with a proper error. It could be a good idea to introduce try_new_null to carry an error gracefully - thoughts?

What changes are included in this PR?

• Return a Result on negative byte width when possible
• Panic explicitly with a proper error message otherwise
• Avoid silent overflow with a direct len as usize cast
• Reject negative FSB when parsing from tokens

Are these changes tested?

• Tests are passing

Are there any user-facing changes?

No

[alamb]

Makes sense to me -- thank you @theirix

[Jefffrey]

Do we have a test case from the original issue? I think DataFusion was calling a parse code path in arrow-rs, so maybe look to add the test case there? (parsing a datatype from string, I mean)

[theirix]

Do we have a test case from the original issue? I think DataFusion was calling a parse code path in arrow-rs, so maybe look to add the test case there? (parsing a datatype from string, I mean)

Not anything I am aware of- can introduce one later. Anyway, writing such a test will be much easier if we have ArrayData::try_new_null - should we? ArrayData::new_null already has a few unwraps inside

[alamb]

Maybe @Jefffrey was talking about

arrow-rs/arrow-schema/src/datatype.rs

Lines 495 to 507 in f1ef71a

	/// Parses `str` into a `DataType`.
	///
	/// This is the reverse of [`DataType`]'s `Display`
	/// impl, and maintains the invariant that
	/// `DataType::try_from(&data_type.to_string()).unwrap() == data_type`
	///
	/// # Example
	/// ```
	/// use arrow_schema::DataType;
	///
	/// let data_type: DataType = "Int32".parse().unwrap();
	/// assert_eq!(data_type, DataType::Int32);
	/// ```

In the sense that it perhaps more likely for someone to be able to feed negative numbers in via SQL than via Rust APIs.

So in this case case, arrow_cast(..) calls DataType::parse which could potentially validate the DataType before return

[theirix]

Thank you for pointing it out. I've added a check into the parser and a test for it.

[alamb]

Is FixedSizeBinary(0) allowed? I think this check only verifies >= 0 🤔

[theirix]

The format doesn't tell anything about it. The C++ implementation allows zero-width buffer. Fixed an error message

[alamb]

Thank you @theirix and @Jefffrey