andygrove opened a new issue, #27068:
URL: https://github.com/apache/datafusion/issues/27068
### Describe the bug
Reading a Parquet file with a top-level BINARY column through a table schema
that declares the column as a string (`Utf8`, `LargeUtf8` or `Utf8View`)
produces a string array that has not been checked for valid UTF-8. That covers
the `binary_as_string` option, and any explicit schema. When a value is not
valid UTF-8:
- in a debug build, the Parquet reader panics in `OffsetBuffer::into_array`
(`parquet/src/arrow/buffer/offset_buffer.rs:139`)
- in a release build, the scan returns a `StringArray` holding invalid UTF-8
(`validate_full()` fails), which is unsound for any kernel that reads the
values as `&str`
`apply_file_schema_type_coercions` rewrites a binary file field to the
table's string type before the Parquet reader runs. The reader validates UTF-8
only for columns annotated `UTF8`, which a BINARY column is not, so nothing
checks the bytes. The module docs on `schema_coercion.rs` already note that
this "hands back an unvalidated string array".
This is long-standing: 55.2.0 behaves the same. It is the top-level form of
#27065. That issue covers nested fields, which regressed in 56 and are fixed in
#27067 by keeping nested binary fields out of the coercion.
### To Reproduce
With `arrow`/`parquet` 60.1.0 and `tokio`:
```rust
use std::sync::Arc;
use arrow::array::{Array, ArrayRef, BinaryArray};
use arrow::datatypes::{DataType, Field, Schema};
use arrow::record_batch::RecordBatch;
use datafusion::prelude::*;
use parquet::arrow::ArrowWriter;
#[tokio::main]
async fn main() -> datafusion::error::Result<()> {
// A top-level binary column whose second value is not valid UTF-8.
let values: ArrayRef =
Arc::new(BinaryArray::from(vec![b"one".as_slice(), &[0xff]]));
let batch = RecordBatch::try_from_iter([("s", values)])?;
let path = std::env::temp_dir().join("top_level_binary.parquet");
let mut writer = ArrowWriter::try_new(std::fs::File::create(&path)?,
batch.schema(), None)?;
writer.write(&batch)?;
writer.close()?;
// Read it with a table schema that declares the column as a string.
let table_schema = Schema::new(vec![Field::new("s", DataType::Utf8,
true)]);
let ctx = SessionContext::new();
ctx.register_parquet(
"t",
path.to_str().unwrap(),
ParquetReadOptions::default().schema(&table_schema),
)
.await?;
let batches = ctx.sql("SELECT s FROM t").await?.collect().await?;
let column = batches[0].column(0);
println!("type: {}", column.data_type());
println!("validate_full: {:?}", column.to_data().validate_full());
Ok(())
}
```
On `branch-56` and on 55.2.0, a debug build panics:
```
called `Result::unwrap()` on an `Err` value: InvalidArgumentError("Invalid
UTF8 sequence at string index 1 (3..4): invalid utf-8 sequence of 1 bytes from
index 0")
```
A release build returns an invalid array:
```
type: Utf8
validate_full: Err(InvalidArgumentError("Invalid UTF8 sequence at string
index 1 (3..4): invalid utf-8 sequence of 1 bytes from index 0"))
```
### Expected behavior
Reading BINARY data as a string never produces an unvalidated string array.
Invalid UTF-8 returns an error, as it already does for nested fields, which go
through the validating cast.
### Additional context
There are two places this could be fixed:
- **In DataFusion**, by not rewriting binary file fields to string types at
all. The column is read as binary, and the schema adapter's cast to the table's
string type validates it. Pruning should be unaffected, since the opener
already moves such casts to the literal side for statistics. For `Utf8View`
tables the file field can be read as `BinaryView`, so the cast only validates
and does not copy.
- **In the Parquet reader**, by validating whenever the target type is a
string type. That is being discussed in apache/arrow-rs#12047, together with an
opt-in `InvalidUtf8::Replace` policy that would let `binary_as_string` users
replace invalid sequences instead of getting an error.
Either way, a query over binary data that is not valid UTF-8, read as a
string, would start returning an error where it currently returns an invalid
array.
### Willingness to contribute
I plan to contribute a fix for this bug independently
--
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]
