alamb commented on code in PR #10206:
URL: https://github.com/apache/arrow-rs/pull/10206#discussion_r3469624284
##########
arrow-flight/src/decode.rs:
##########
@@ -18,6 +18,8 @@
use crate::{FlightData, trailers::LazyTrailers,
utils::flight_data_to_arrow_batch};
use arrow_array::{ArrayRef, RecordBatch};
use arrow_buffer::Buffer;
+use arrow_data::UnsafeFlag;
+//use arrow_ipc::reader;
Review Comment:
I think we can remove this
```suggestion
```
##########
arrow-flight/Cargo.toml:
##########
@@ -32,7 +32,7 @@ arrow-array = { workspace = true }
arrow-buffer = { workspace = true }
# Cast is needed to work around https://github.com/apache/arrow-rs/issues/3389
arrow-cast = { workspace = true }
-arrow-data = { workspace = true, optional = true }
+arrow-data = { workspace = true }
Review Comment:
I think this is ok as arrow-cast already brings in this dependency
##########
arrow-flight/src/decode.rs:
##########
@@ -250,9 +255,17 @@ impl FlightDataDecoder {
state: None,
response: response.boxed(),
done: false,
+ skip_validation: UnsafeFlag::new(),
}
}
+ /// # Safety
Review Comment:
Can we also leave a comment to the docs in the IPC deocder that talks more
about the implications of skipping validation?
##########
arrow-ipc/src/reader.rs:
##########
@@ -771,11 +771,13 @@ pub fn read_record_batch(
dictionaries_by_id: &HashMap<i64, ArrayRef>,
projection: Option<&[usize]>,
metadata: &MetadataVersion,
+ skip_validation: UnsafeFlag,
Review Comment:
Since this is a public API it means we can't release this until the next
major (breaking) API release
https://docs.rs/arrow-ipc/latest/arrow_ipc/reader/fn.read_record_batch.html
do we really need to make this change? Perhaps we should just direct people
to use `RecordBatchDecoder` directly if they want more control over the
decoding process rather than changing this signature.
However, not much of that strucutre seems to be public at the moment:
https://docs.rs/arrow-ipc/latest/arrow_ipc/reader/struct.RecordBatchDecoder.html
##########
arrow-ipc/src/reader.rs:
##########
@@ -771,11 +771,13 @@ pub fn read_record_batch(
dictionaries_by_id: &HashMap<i64, ArrayRef>,
projection: Option<&[usize]>,
metadata: &MetadataVersion,
+ skip_validation: UnsafeFlag,
) -> Result<RecordBatch, ArrowError> {
- RecordBatchDecoder::try_new(buf, batch, schema, dictionaries_by_id,
metadata)?
+ let decoder = RecordBatchDecoder::try_new(buf, batch, schema,
dictionaries_by_id, metadata)?
Review Comment:
nit: we can probably make this more concise by not adding a `let decoder =
...`
--
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]
