laskoviymishka commented on code in PR #3080:
URL: https://github.com/apache/iceberg-rust/pull/3080#discussion_r3866194789
##########
crates/iceberg/src/arrow/value.rs:
##########
@@ -864,24 +864,24 @@ pub(crate) fn create_primitive_array_repeated(
(DataType::Float64, Some(PrimitiveLiteral::Double(value))) => {
Arc::new(Float64Array::from(vec![value.0; num_rows]))
}
- (DataType::Utf8, Some(PrimitiveLiteral::String(value))) => {
- Arc::new(StringArray::from(vec![value.clone(); num_rows]))
- }
- (DataType::Binary, Some(PrimitiveLiteral::Binary(value))) => {
- Arc::new(BinaryArray::from_vec(vec![value; num_rows]))
- }
- (DataType::LargeBinary, Some(PrimitiveLiteral::Binary(value))) => {
- Arc::new(LargeBinaryArray::from_vec(vec![value; num_rows]))
- }
- (DataType::FixedSizeBinary(len),
Some(PrimitiveLiteral::Binary(value))) => {
- let repeated: Vec<&[u8]> = vec![value.as_slice(); num_rows];
-
Arc::new(FixedSizeBinaryArray::try_from_iter(repeated.into_iter()).map_err(|e| {
- Error::new(
- ErrorKind::DataInvalid,
- format!("Failed to create FixedSizeBinary({len}) array:
{e}"),
- )
- })?)
- }
+ (DataType::Utf8, Some(PrimitiveLiteral::String(value))) => Arc::new(
+ StringArray::from_iter_values(std::iter::repeat_n(value.as_str(),
num_rows)),
+ ),
+ (DataType::Binary, Some(PrimitiveLiteral::Binary(value))) => Arc::new(
+
BinaryArray::from_iter_values(std::iter::repeat_n(value.as_slice(), num_rows)),
+ ),
+ (DataType::LargeBinary, Some(PrimitiveLiteral::Binary(value))) =>
Arc::new(
+
LargeBinaryArray::from_iter_values(std::iter::repeat_n(value.as_slice(),
num_rows)),
+ ),
+ (DataType::FixedSizeBinary(len),
Some(PrimitiveLiteral::Binary(value))) => Arc::new(
+
FixedSizeBinaryArray::try_from_iter(std::iter::repeat_n(value.as_slice(),
num_rows))
Review Comment:
I think there's a subtle edge worth nailing down while we're touching this
arm. `try_from_iter` infers the fixed width from the data it's handed, not from
the `len` we matched on — so with `num_rows == 0` the iterator is empty and we
get back a `FixedSizeBinary(0)` array no matter what the schema declared, and
an empty batch over a `fixed[n]` partition column (any filter that drops all
rows) would then hand a type-mismatched array to `RecordBatch::try_new`. Same
story if a literal's length doesn't equal `*len` — `len` only feeds the error
string today, so a wrong-width value silently produces the wrong type rather
than erroring.
Both are pre-existing rather than introduced here, but the new empty-batch
test only covers `Utf8`. Extending `test_create_string_array_repeated_empty` to
`FixedSizeBinary(4) + num_rows=0` and asserting `data_type()` would either
prove it's fine or pin it down, and a `value.len() == *len` guard before
`try_from_iter` would close the wrong-width case. wdyt?
--
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]
