alamb commented on code in PR #6433:
URL: https://github.com/apache/arrow-rs/pull/6433#discussion_r1769529121
##########
arrow-array/src/builder/generic_bytes_builder.rs:
##########
@@ -537,7 +537,7 @@ mod tests {
write!(builder, "buz").unwrap();
builder.append_value("");
let a = builder.finish();
- let r: Vec<_> = a.iter().map(|x| x.unwrap()).collect();
+ let r: Vec<_> = a.iter().flatten().collect();
Review Comment:
I think this change is semantically different (won't panic, but would be
smaller array) but still preserves the intent of the test
##########
arrow-schema/src/ffi.rs:
##########
@@ -274,35 +282,59 @@ impl FFI_ArrowSchema {
}
}
+ /// Returns the flags of this schema.
pub fn flags(&self) -> Option<Flags> {
Flags::from_bits(self.flags)
}
+ /// Returns the child of this schema at `index`.
+ ///
+ /// # Panics
+ ///
+ /// Panics if `index` is greater than or equal to the number of children.
+ ///
+ /// This is to make sure that the unsafe acces to raw pointer is sound.
+ ///
+ /// # Safety
Review Comment:
I don't think this needs a safety section as the function is not marked
`unsafe` and if an invalid index is provided the code will panic rather than
produce undefined behavior
##########
arrow-csv/src/writer.rs:
##########
@@ -508,7 +509,7 @@ Lorem ipsum dolor sit
amet,123.564532,3,true,,00:20:34,cupcakes
consectetur adipiscing elit,,2,false,2019-04-18T10:54:47.378,06:51:20,cupcakes
sed do eiusmod tempor,-556132.25,1,,2019-04-18T02:45:55.555,23:46:03,foo
"#;
- assert_eq!(expected.to_string(), String::from_utf8(buffer).unwrap());
+ assert_eq!(expected.to_string(), str::from_utf8(&buffer).unwrap());
Review Comment:
maybe expected doesn't need `to_string` either 🤔
##########
arrow-schema/src/ffi.rs:
##########
@@ -37,25 +37,27 @@
use crate::{
ArrowError, DataType, Field, FieldRef, IntervalUnit, Schema, TimeUnit,
UnionFields, UnionMode,
};
+use bitflags::bitflags;
use std::sync::Arc;
use std::{
collections::HashMap,
ffi::{c_char, c_void, CStr, CString},
};
-#[allow(clippy::assign_op_pattern)]
-/// Workaround <https://github.com/bitflags/bitflags/issues/356>
-mod flags {
- use bitflags::bitflags;
- bitflags! {
- pub struct Flags: i64 {
- const DICTIONARY_ORDERED = 0b00000001;
- const NULLABLE = 0b00000010;
- const MAP_KEYS_SORTED = 0b00000100;
- }
+bitflags! {
Review Comment:
I double checked this looks the same to me:
https://docs.rs/arrow-schema/53.0.0/arrow_schema/ffi/struct.Flags.html
👍
##########
arrow-schema/src/ffi.rs:
##########
@@ -70,10 +72,12 @@ pub use flags::*;
///
#[repr(C)]
#[derive(Debug)]
+#[allow(non_camel_case_types)]
pub struct FFI_ArrowSchema {
format: *const c_char,
name: *const c_char,
metadata: *const c_char,
+ /// Refer to [Arrow
Flags](https://arrow.apache.org/docs/format/CDataInterface.html#c.ArrowSchema.flags)
Review Comment:
👍
--
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]
