sdf-jkl commented on code in PR #10114:
URL: https://github.com/apache/arrow-rs/pull/10114#discussion_r3908319435
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -708,6 +711,125 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+fn write_utc_timestamp_with_default_format(
+ f: &mut dyn Write,
+ naive: NaiveDateTime,
+ timezone: Option<Tz>,
+) -> FormatResult {
+ match timezone {
+ Some(tz) => {
+ let date = Utc.from_utc_datetime(&naive).with_timezone(&tz);
+ write!(f, "{}", date.to_rfc3339_opts(SecondsFormat::AutoSi, true))?
+ }
+ None => write!(f, "{naive:?}")?
+ }
+ Ok(())
+}
+
+// convert a variant to an owned string.
+pub(crate) fn variant_to_string(variant: &Variant<'_, '_>) -> Option<String> {
+ match variant {
+ Variant::String(s) => Some(s.to_string()),
+ Variant::ShortString(s) => Some(s.to_string()),
+ Variant::BooleanTrue => Some("true".into()),
+ Variant::BooleanFalse => Some("false".into()),
+ Variant::Int8(i) => Some(lexical_to_string(*i)),
+ Variant::Int16(i) => Some(lexical_to_string(*i)),
+ Variant::Int32(i) => Some(lexical_to_string(*i)),
+ Variant::Int64(i) => Some(lexical_to_string(*i)),
+ Variant::Float(f) => Some(lexical_to_string(*f)),
Review Comment:
`arrow-cast`'s `ArrayFormatter` uses `ryu` for float
https://github.com/apache/arrow-rs/blob/d5cd0da2f2d2c2118b25980e8c0f41d6f935fa2f/arrow-cast/src/display.rs#L712-L724
the difference:
```rust
#[test]
fn reproduce_float_string_difference() {
let value = f32::from_bits(0xd378_62a7);
// What #10114 currently produces
let variant_output = lexical_to_string(value);
// What arrow-cast produces
let array = Float32Array::from(vec![value]);
let casted = cast(&array, &DataType::Utf8).unwrap();
let arrow_output = casted.as_string::<i32>().value(0);
assert_eq!(variant_output, "-1.066807e12");
assert_eq!(arrow_output, "-1066807000000.0");
// Fails:
assert_eq!(variant_output, arrow_output);
}
```
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -708,6 +711,125 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+fn write_utc_timestamp_with_default_format(
+ f: &mut dyn Write,
+ naive: NaiveDateTime,
+ timezone: Option<Tz>,
+) -> FormatResult {
+ match timezone {
+ Some(tz) => {
+ let date = Utc.from_utc_datetime(&naive).with_timezone(&tz);
+ write!(f, "{}", date.to_rfc3339_opts(SecondsFormat::AutoSi, true))?
+ }
+ None => write!(f, "{naive:?}")?
+ }
+ Ok(())
+}
+
+// convert a variant to an owned string.
+pub(crate) fn variant_to_string(variant: &Variant<'_, '_>) -> Option<String> {
+ match variant {
+ Variant::String(s) => Some(s.to_string()),
+ Variant::ShortString(s) => Some(s.to_string()),
+ Variant::BooleanTrue => Some("true".into()),
+ Variant::BooleanFalse => Some("false".into()),
+ Variant::Int8(i) => Some(lexical_to_string(*i)),
+ Variant::Int16(i) => Some(lexical_to_string(*i)),
+ Variant::Int32(i) => Some(lexical_to_string(*i)),
+ Variant::Int64(i) => Some(lexical_to_string(*i)),
+ Variant::Float(f) => Some(lexical_to_string(*f)),
+ Variant::Double(f) => Some(lexical_to_string(*f)),
+ Variant::Decimal4(d) => {
+ let value_str = d.integer().to_string();
+ Some(format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ))
+ }
+ Variant::Decimal8(d) => {
+ let value_str = d.integer().to_string();
+ Some(format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ))
+ }
+ Variant::Decimal16(d) => {
+ let value_str = d.integer().to_string();
+ Some(format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ))
+ }
+ Variant::Date(d) => {
+ let mut ret_string = String::new();
+ let _ = write!(ret_string, "{d:?}");
+ Some(ret_string)
+ }
+ Variant::Time(t) => {
+ let mut ret_string = String::new();
+ let _ = write!(ret_string, "{t:?}");
+ Some(ret_string)
+ }
+ Variant::TimestampMicros(t) => {
+ let mut out = String::new();
+ let _ = write_utc_timestamp_with_default_format(&mut out,
t.naive_utc(), "+00:00".parse().ok());
+ Some(out)
+ }
+ Variant::TimestampNtzMicros(t) => {
+ let mut out = String::new();
+ let _ = write_utc_timestamp_with_default_format(&mut out, *t,
None);
+ Some(out)
+ }
+ Variant::TimestampNanos(t) => {
+ let mut out = String::new();
+ let _ = write_utc_timestamp_with_default_format(&mut out,
t.naive_utc(), "+00:00".parse().ok());
+ Some(out)
+ }
+ Variant::TimestampNtzNanos(t) => {
+ let mut out = String::new();
+ let _ = write_utc_timestamp_with_default_format(&mut out, *t,
None);
+ Some(out)
+ }
+ Variant::Uuid(u) => Some(u.to_string()),
+ Variant::Binary(v) => std::str::from_utf8(v).ok().map(|s|
s.to_string()),
+ Variant::List(l) => Some(cast_list_to_string(l.iter())),
+ _ => None,
+ }
+}
+
+fn cast_list_to_string<'m, 'v>(mut iter: impl Iterator<Item = Variant<'m,
'v>>) -> String {
+ let mut ret_str = String::new();
+ let _ = ret_str.write_char('[');
+
+ if let Some(item) = iter.next() {
+ let _ = write!(ret_str, "{}",
variant_to_string(&item).unwrap_or_default());
+ }
+
+ for item in iter {
+ let _ = write!(
+ ret_str,
+ ", {}",
+ variant_to_string(&item).unwrap_or_default()
Review Comment:
By calling `variant_to_string` recursively we drop the `Variant::Object`
values inside a list.
arrow-cast doesn't support Objects -> String, but supports them inside a
List [via
`ArrayFormatter`](https://github.com/apache/arrow-rs/blob/70219af2ffa16615e2fcde5760b0218f5c986ac6/arrow-cast/src/display.rs#L1244-L1261)
🤷 (separate issue maybe?)
regardless of correctness of the Object support above, variant-cast to
String shouldn't lose data.
```rust
#[test]
fn reproduce_list_of_objects_utf8_difference() {
// Build the Variant value: [{"x": 1}]
let mut variant_builder = VariantBuilder::new();
let mut variant_list = variant_builder.new_list();
variant_list
.new_object()
.with_field("x", 1_i32)
.finish();
variant_list.finish();
let (metadata, value) = variant_builder.finish();
let variant = Variant::new(&metadata, &value);
// Current #10114 result
let variant_output = variant_to_string(&variant).unwrap();
// Build the equivalent Arrow List<Struct<x: Int32>>
let fields = vec![Field::new("x", DataType::Int32, true)];
let struct_builder = StructBuilder::from_fields(fields, 1);
let mut arrow_list = ListBuilder::new(struct_builder);
let struct_builder = arrow_list.values();
struct_builder
.field_builder::<Int32Builder>(0)
.unwrap()
.append_value(1);
struct_builder.append(true);
arrow_list.append(true);
let arrow_list = arrow_list.finish();
// Arrow List<Struct> → Utf8 result
let casted = cast(&arrow_list, &DataType::Utf8).unwrap();
let arrow_output = casted.as_string::<i32>().value(0);
assert_eq!(variant_output, "[]");
assert_eq!(arrow_output, "[{x: 1}]");
// Fails
assert_eq!(variant_output, arrow_output);
}
```
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -708,6 +711,125 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+fn write_utc_timestamp_with_default_format(
+ f: &mut dyn Write,
+ naive: NaiveDateTime,
+ timezone: Option<Tz>,
+) -> FormatResult {
+ match timezone {
+ Some(tz) => {
+ let date = Utc.from_utc_datetime(&naive).with_timezone(&tz);
+ write!(f, "{}", date.to_rfc3339_opts(SecondsFormat::AutoSi, true))?
+ }
+ None => write!(f, "{naive:?}")?
+ }
+ Ok(())
+}
+
+// convert a variant to an owned string.
+pub(crate) fn variant_to_string(variant: &Variant<'_, '_>) -> Option<String> {
Review Comment:
We should add
[`FormatOptions`](https://github.com/apache/arrow-rs/blob/d5cd0da2f2d2c2118b25980e8c0f41d6f935fa2f/arrow-cast/src/display.rs#L73-L110)
parameter to pick how to write `Date/Time/Timestamps` to a string. It
shouldn't be just default like it is in
`write_utc_timestamp_with_default_format` and `Date/Time` arms.
--
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]
