sdf-jkl commented on code in PR #10114:
URL: https://github.com/apache/arrow-rs/pull/10114#discussion_r4125140022
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -705,6 +710,227 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+#[inline]
+fn write_float_to_string<F: Float>(f: F, out: &mut String) {
+ let mut buffer = ryu::Buffer::new();
+ out.push_str(buffer.format(f));
+}
+
+// Convert a variant to a borrowed string when possible, otherwise an owned
formatted string.
+pub(crate) fn variant_to_string<'v>(
+ variant: &Variant<'_, 'v>,
+ formats: &TemporalFormats<'_>,
+) -> Option<Cow<'v, str>> {
+ match variant {
+ Variant::Null => None,
+ Variant::Object(_) => None,
+ Variant::String(s) => Some(Cow::Borrowed(s)),
+ Variant::ShortString(s) => Some(Cow::Borrowed(s.as_str())),
+ _ => {
+ let mut s = String::new();
+ write_variant_to_string(variant, formats, &mut
s).then_some(Cow::Owned(s))
+ }
+ }
+}
+
+fn write_lexical_to_string<N: lexical_core::ToLexical>(out: &mut String, n: N)
{
+ // i64::FORMATTED_SIZE is the upper bound for all integer types we support
+ // (i8/i16/i32/i64). With power-of-two feature it's 128, otherwise 20.
+ // We can't use N::FORMATTED_SIZE in a generic function
(generic_const_exprs).
+ let mut buf = [0u8; i64::FORMATTED_SIZE];
+ let written = lexical_core::write(n, &mut buf);
+ // Lexical core produces valid UTF-8
+ out.push_str(unsafe { std::str::from_utf8_unchecked(written) });
+}
+
+fn write_variant_to_string(
+ variant: &Variant<'_, '_>,
+ formats: &TemporalFormats<'_>,
+ out: &mut String,
+) -> bool {
+ match variant {
+ Variant::Null => {
+ out.push_str(formats.null());
+ true
+ }
+ Variant::String(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::ShortString(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::BooleanTrue => {
+ out.push_str("true");
+ true
+ }
+ Variant::BooleanFalse => {
+ out.push_str("false");
+ true
+ }
+ Variant::Int8(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int16(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int32(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int64(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Float(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Double(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Decimal4(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
Review Comment:
Could we use `write_decimal` here and in the `Decimal8`/`Decimal16` arms? It
was added in #11002 and is already available in this PR's base through
`arrow::datatypes`. It formats the integer using a stack buffer and writes
directly into `out`, avoiding the two temporary heap strings from
`integer().to_string()` and `format_decimal_str()` for every decimal value.
```suggestion
arrow::datatypes::write_decimal(out, d.integer(), d.scale() as
_).is_ok()
```
This snippet fits the current `bool` return type. If we adopt the separate
`Result` suggestion, propagate the write result with `?` instead of `.is_ok()`.
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -204,6 +205,62 @@ pub(crate) fn make_variant_to_arrow_row_builder<'a>(
Ok(builder)
}
+pub(crate) enum VariantToStringArrowRowBuilder<'a, B: StringLikeArrayBuilder> {
+ Get(VariantToStringGetArrowBuilder<'a, B>),
+ Shred(VariantToStringShredArrowBuilder<'a, B>),
+}
Review Comment:
Could we follow the existing Boolean/numeric/timestamp/decimal builder
pattern here: keep one string builder and one binary builder, each taking
`shred: bool`, and drop the separate Get/Shred implementations and forwarding
enums?
For binary, the shared builder can select `value.as_u8_slice()` when
shredding and `variant_to_binary(value)` otherwise. For strings, both modes can
share the existing String/ShortString fast path; other values should only go
through string formatting when `shred` is false.
This preserves strict shredding behavior while removing the extra builder
types, forwarding methods, and repeated constructor branches for the six
string/binary array types.
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -705,6 +710,227 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+#[inline]
+fn write_float_to_string<F: Float>(f: F, out: &mut String) {
+ let mut buffer = ryu::Buffer::new();
+ out.push_str(buffer.format(f));
+}
+
+// Convert a variant to a borrowed string when possible, otherwise an owned
formatted string.
+pub(crate) fn variant_to_string<'v>(
+ variant: &Variant<'_, 'v>,
+ formats: &TemporalFormats<'_>,
+) -> Option<Cow<'v, str>> {
+ match variant {
+ Variant::Null => None,
+ Variant::Object(_) => None,
+ Variant::String(s) => Some(Cow::Borrowed(s)),
+ Variant::ShortString(s) => Some(Cow::Borrowed(s.as_str())),
+ _ => {
+ let mut s = String::new();
+ write_variant_to_string(variant, formats, &mut
s).then_some(Cow::Owned(s))
+ }
+ }
+}
+
+fn write_lexical_to_string<N: lexical_core::ToLexical>(out: &mut String, n: N)
{
+ // i64::FORMATTED_SIZE is the upper bound for all integer types we support
+ // (i8/i16/i32/i64). With power-of-two feature it's 128, otherwise 20.
+ // We can't use N::FORMATTED_SIZE in a generic function
(generic_const_exprs).
+ let mut buf = [0u8; i64::FORMATTED_SIZE];
+ let written = lexical_core::write(n, &mut buf);
+ // Lexical core produces valid UTF-8
+ out.push_str(unsafe { std::str::from_utf8_unchecked(written) });
+}
+
+fn write_variant_to_string(
+ variant: &Variant<'_, '_>,
+ formats: &TemporalFormats<'_>,
+ out: &mut String,
+) -> bool {
+ match variant {
+ Variant::Null => {
+ out.push_str(formats.null());
+ true
+ }
+ Variant::String(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::ShortString(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::BooleanTrue => {
+ out.push_str("true");
+ true
+ }
+ Variant::BooleanFalse => {
+ out.push_str("false");
+ true
+ }
+ Variant::Int8(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int16(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int32(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int64(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Float(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Double(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Decimal4(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Decimal8(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Decimal16(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Date(d) => {
+ // The writing is always success
+ write_temporal_display(out, d, formats.date()).is_ok()
+ }
+ Variant::Time(t) => {
+ // The writing is always success
+ write_temporal_display(out, t, formats.time()).is_ok()
+ }
+ Variant::TimestampMicros(t) => {
+ // The writing is always success
+ write_timestamp(
+ out,
+ t.naive_utc(),
+ "+00:00".parse().ok(),
+ formats.timestamp_tz(),
+ )
+ .is_ok()
+ }
+ Variant::TimestampNtzMicros(t) => {
+ // The writing is always success
+ write_timestamp(out, *t, None, formats.timestamp()).is_ok()
+ }
+ Variant::TimestampNanos(t) => {
+ // The writing is always success
+ write_timestamp(
+ out,
+ t.naive_utc(),
+ "+00:00".parse().ok(),
+ formats.timestamp_tz(),
+ )
+ .is_ok()
+ }
+ Variant::TimestampNtzNanos(t) => {
+ // The writing is always success
+ write_timestamp(out, *t, None, formats.timestamp()).is_ok()
+ }
+ Variant::Uuid(u) => write!(out, "{u}").is_ok(),
+ Variant::Binary(v) => match std::str::from_utf8(v) {
+ Ok(s) => {
+ out.push_str(s);
+ true
+ }
+ Err(_) => false,
Review Comment:
Could we use Arrow's hex formatting for binary values nested in collections?
A list containing `b"ab"` and `[0xff, 0x00]` currently becomes `[ab, ]`,
whereas Arrow produces `[6162, ff00]`. Please add a comparison test for this
case.
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -1273,8 +1419,64 @@ macro_rules! define_variant_to_primitive_builder {
}
}
+pub(crate) struct VariantToStringGetArrowBuilder<'a, B:
StringLikeArrayBuilder> {
+ builder: B,
+ cast_options: &'a CastOptions<'a>,
+ formats: TemporalFormats<'a>,
+}
+
+impl<'a, B: StringLikeArrayBuilder> VariantToStringGetArrowBuilder<'a, B> {
+ fn new(cast_options: &'a CastOptions<'a>, capacity: usize) -> Self {
+ Self {
+ builder: B::with_capacity(capacity),
+ cast_options,
+ formats: TemporalFormats::new(cast_options),
+ }
+ }
+
+ fn append_null(&mut self) -> Result<()> {
+ self.builder.append_null();
+ Ok(())
+ }
+
+ fn append_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ let value = match value {
+ // short-circuit the common case of string-like variants to avoid
the overhead of casting
+ Variant::String(value) => value,
+ Variant::ShortString(value) => value.as_str(),
+ _ => return self.append_cast_value(value),
+ };
+
+ self.builder.append_value(value);
+ Ok(true)
+ }
+
+ fn append_cast_value(&mut self, value: &Variant<'_, '_>) -> Result<bool> {
+ match variant_cast_with_options(value, self.cast_options, |value| {
+ variant_to_string(value, &self.formats)
Review Comment:
A perf suggestion, can bench before and after.
We can have a reusable `scratch: String` on the string builder.
`variant_to_string` currently creates a fresh `String` for each formatted
value, which is then copied into the Arrow builder and dropped.
Calling `scratch.clear()` before each conversion, writing through
`write_variant_to_string`, and appending `scratch.as_str()` on success would
reuse the capacity across rows. The String/ShortString fast path can stay
as-is. We would need to make the writer `pub(crate)` and preserve the wrapper's
rejection of top-level Null/Object values (and the strict shredding guard if
the builders are consolidated).
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -1204,16 +1302,61 @@ impl VariantPathRowBuilder<'_> {
}
}
+#[derive(Default)]
+pub(crate) struct TemporalFormats<'a> {
+ date: CompiledTimeFormat<'a>,
+ time: CompiledTimeFormat<'a>,
+ timestamp: CompiledTimeFormat<'a>,
+ timestamp_tz: CompiledTimeFormat<'a>,
+ null: &'a str,
+}
Review Comment:
Optional cleanup: could we move `TemporalFormats` beside the formatting
functions in `type_conversion.rs`? That would remove the reverse dependency
from the conversion module to the builder module and let the formatter access
these fields directly, without the five forwarding getters.
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -705,6 +710,227 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+#[inline]
+fn write_float_to_string<F: Float>(f: F, out: &mut String) {
+ let mut buffer = ryu::Buffer::new();
+ out.push_str(buffer.format(f));
+}
+
+// Convert a variant to a borrowed string when possible, otherwise an owned
formatted string.
+pub(crate) fn variant_to_string<'v>(
+ variant: &Variant<'_, 'v>,
+ formats: &TemporalFormats<'_>,
+) -> Option<Cow<'v, str>> {
+ match variant {
+ Variant::Null => None,
+ Variant::Object(_) => None,
+ Variant::String(s) => Some(Cow::Borrowed(s)),
+ Variant::ShortString(s) => Some(Cow::Borrowed(s.as_str())),
+ _ => {
+ let mut s = String::new();
+ write_variant_to_string(variant, formats, &mut
s).then_some(Cow::Owned(s))
+ }
+ }
+}
+
+fn write_lexical_to_string<N: lexical_core::ToLexical>(out: &mut String, n: N)
{
+ // i64::FORMATTED_SIZE is the upper bound for all integer types we support
+ // (i8/i16/i32/i64). With power-of-two feature it's 128, otherwise 20.
+ // We can't use N::FORMATTED_SIZE in a generic function
(generic_const_exprs).
+ let mut buf = [0u8; i64::FORMATTED_SIZE];
+ let written = lexical_core::write(n, &mut buf);
+ // Lexical core produces valid UTF-8
+ out.push_str(unsafe { std::str::from_utf8_unchecked(written) });
+}
+
+fn write_variant_to_string(
+ variant: &Variant<'_, '_>,
+ formats: &TemporalFormats<'_>,
+ out: &mut String,
+) -> bool {
+ match variant {
+ Variant::Null => {
+ out.push_str(formats.null());
+ true
+ }
+ Variant::String(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::ShortString(s) => {
+ out.push_str(s);
+ true
Review Comment:
Could we honor `FormatOptions::with_quoted_strings(true)` when formatting
nested strings? These branches always write raw text, so lists containing
commas, quotes, or newlines differ from Arrow. Please pass the option through
and add a comparison test.
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -705,6 +710,227 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+#[inline]
+fn write_float_to_string<F: Float>(f: F, out: &mut String) {
+ let mut buffer = ryu::Buffer::new();
+ out.push_str(buffer.format(f));
+}
+
+// Convert a variant to a borrowed string when possible, otherwise an owned
formatted string.
+pub(crate) fn variant_to_string<'v>(
+ variant: &Variant<'_, 'v>,
+ formats: &TemporalFormats<'_>,
+) -> Option<Cow<'v, str>> {
+ match variant {
+ Variant::Null => None,
+ Variant::Object(_) => None,
+ Variant::String(s) => Some(Cow::Borrowed(s)),
+ Variant::ShortString(s) => Some(Cow::Borrowed(s.as_str())),
+ _ => {
+ let mut s = String::new();
+ write_variant_to_string(variant, formats, &mut
s).then_some(Cow::Owned(s))
+ }
+ }
+}
+
+fn write_lexical_to_string<N: lexical_core::ToLexical>(out: &mut String, n: N)
{
+ // i64::FORMATTED_SIZE is the upper bound for all integer types we support
+ // (i8/i16/i32/i64). With power-of-two feature it's 128, otherwise 20.
+ // We can't use N::FORMATTED_SIZE in a generic function
(generic_const_exprs).
+ let mut buf = [0u8; i64::FORMATTED_SIZE];
+ let written = lexical_core::write(n, &mut buf);
+ // Lexical core produces valid UTF-8
+ out.push_str(unsafe { std::str::from_utf8_unchecked(written) });
+}
+
+fn write_variant_to_string(
+ variant: &Variant<'_, '_>,
+ formats: &TemporalFormats<'_>,
+ out: &mut String,
+) -> bool {
+ match variant {
+ Variant::Null => {
+ out.push_str(formats.null());
+ true
+ }
+ Variant::String(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::ShortString(s) => {
+ out.push_str(s);
+ true
+ }
+ Variant::BooleanTrue => {
+ out.push_str("true");
+ true
+ }
+ Variant::BooleanFalse => {
+ out.push_str("false");
+ true
+ }
+ Variant::Int8(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int16(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int32(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Int64(i) => {
+ write_lexical_to_string(out, *i);
+ true
+ }
+ Variant::Float(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Double(f) => {
+ write_float_to_string(*f, out);
+ true
+ }
+ Variant::Decimal4(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Decimal8(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Decimal16(d) => {
+ let value_str = d.integer().to_string();
+ out.push_str(&format_decimal_str(
+ &value_str,
+ value_str.len(),
+ d.scale() as _,
+ ));
+ true
+ }
+ Variant::Date(d) => {
+ // The writing is always success
+ write_temporal_display(out, d, formats.date()).is_ok()
+ }
+ Variant::Time(t) => {
+ // The writing is always success
+ write_temporal_display(out, t, formats.time()).is_ok()
+ }
+ Variant::TimestampMicros(t) => {
+ // The writing is always success
+ write_timestamp(
+ out,
+ t.naive_utc(),
+ "+00:00".parse().ok(),
+ formats.timestamp_tz(),
+ )
+ .is_ok()
+ }
+ Variant::TimestampNtzMicros(t) => {
+ // The writing is always success
+ write_timestamp(out, *t, None, formats.timestamp()).is_ok()
+ }
+ Variant::TimestampNanos(t) => {
+ // The writing is always success
+ write_timestamp(
+ out,
+ t.naive_utc(),
+ "+00:00".parse().ok(),
+ formats.timestamp_tz(),
+ )
+ .is_ok()
+ }
+ Variant::TimestampNtzNanos(t) => {
+ // The writing is always success
+ write_timestamp(out, *t, None, formats.timestamp()).is_ok()
+ }
+ Variant::Uuid(u) => write!(out, "{u}").is_ok(),
+ Variant::Binary(v) => match std::str::from_utf8(v) {
+ Ok(s) => {
+ out.push_str(s);
+ true
+ }
+ Err(_) => false,
+ },
+ Variant::List(l) => {
+ write_list_to_string(l.iter(), formats, out);
+ true
+ }
+ Variant::Object(o) => {
+ write_map_to_string(o.iter(), formats, out);
+ true
+ }
+ }
+}
+
+fn write_list_to_string<'m, 'v>(
+ mut iter: impl Iterator<Item = Variant<'m, 'v>>,
+ formats: &TemporalFormats,
+ out: &mut String,
+) {
+ out.push('[');
+ if let Some(item) = iter.next()
+ && !write_variant_to_string(&item, formats, out)
+ {
+ out.push_str(formats.null());
Review Comment:
Could we return `Result` from the recursive formatter and use `?`, rather
than reducing formatting errors to `bool` and replacing failed children with
null text? A date inside a list with `%Y-%m-%d %H` currently succeeds even with
`safe: false`; the same format on a top-level date returns NULL with `safe:
true`, while Arrow errors in both cases. Keeping formatting failures distinct
from unsupported casts would fix both cases and remove the repeated
`true`/`.is_ok()` branches. Please cover both cases with regression tests.
##########
arrow-cast/src/display.rs:
##########
@@ -817,6 +831,54 @@ timestamp_display!(
TimestampNanosecondType
);
+/// A temporal value that can be formatted with a [`CompiledTimeFormat`].
+///
+/// This trait is implemented for [`NaiveDate`], [`NaiveTime`], and
[`NaiveDateTime`].
+/// It enables [`write_temporal_display`] to format these types according to a
specified time format.
+pub trait TemporalFormat: Debug {
+ /// Writes this value using the given compiled time format.
+ fn write_with_time_format(
+ &self,
+ f: &mut dyn Write,
+ format: &CompiledTimeFormat<'_>,
+ ) -> FormatResult;
+}
+
+macro_rules! impl_temporal_format {
+ ($($t:ty),+ $(,)?) => {
+ $(
+ impl TemporalFormat for $t {
+ #[inline]
+ fn write_with_time_format(
+ &self,
+ f: &mut dyn Write,
+ format: &CompiledTimeFormat<'_>,
+ ) -> FormatResult {
+ match &format.0 {
+ CompiledTimeFormatInner::Custom(items) => {
+ write!(f, "{}",
self.format_with_items(items.0.iter()))?
Review Comment:
Could we use `self.format_with_items(items.0.iter()).write_to(f)?` here, and
the equivalent in the custom timestamp branches? Chrono's `Display`
implementation allocates an intermediate String even when the destination
buffer is reused; `write_to` writes directly to it. It can leave partial output
on failure, so we should propagate the error and cover invalid formats, as
discussed in the error-handling comment.
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -762,4 +988,580 @@ macro_rules! primitive_conversion_single_value {
)
}};
}
+use crate::variant_to_arrow::TemporalFormats;
pub(crate) use primitive_conversion_single_value;
+
+#[cfg(test)]
+mod tests {
+ use crate::type_conversion::variant_to_string;
+ use crate::variant_to_arrow::TemporalFormats;
+ use arrow::array::{
+ Array, AsArray, BooleanArray, Date32Array, Float32Array, Float64Array,
Int32Builder,
+ ListBuilder, MapBuilder, StringBuilder, Time64MicrosecondArray,
TimestampMicrosecondArray,
+ TimestampNanosecondArray,
+ };
+ use arrow::compute::{CastOptions, cast, cast_with_options};
+ use arrow::util::display::FormatOptions;
+ use arrow_schema::DataType;
+ use chrono::{DateTime, NaiveDate, NaiveTime};
+ use parquet_variant::{Variant, VariantBuilder, VariantBuilderExt};
+ use std::borrow::Cow;
+ use std::iter::zip;
+
+ #[test]
+ fn test_compatible_cast_logic_with_cast_kernel() {
Review Comment:
Optional test cleanup: could we factor the repeated comparison setup into a
helper that runs `cast_to_variant` -> `variant_get` and compares against Arrow
casting? Reusing it for Utf8/LargeUtf8/Utf8View, nulls, and safe/strict
failures would shorten these tests while covering the builder path as well as
the formatter.
##########
parquet-variant-compute/src/type_conversion.rs:
##########
@@ -705,6 +710,227 @@ pub(crate) fn variant_to_boolean(variant: &Variant<'_,
'_>, shred: bool) -> Opti
}
}
+#[inline]
+fn write_float_to_string<F: Float>(f: F, out: &mut String) {
+ let mut buffer = ryu::Buffer::new();
+ out.push_str(buffer.format(f));
+}
+
+// Convert a variant to a borrowed string when possible, otherwise an owned
formatted string.
+pub(crate) fn variant_to_string<'v>(
+ variant: &Variant<'_, 'v>,
+ formats: &TemporalFormats<'_>,
+) -> Option<Cow<'v, str>> {
Review Comment:
If we adopt the scratch-buffer suggestion, could we remove this allocating
wrapper and its `Cow` machinery? The builder can use `write_variant_to_string`
directly, retaining the string fast path and top-level Null/Object checks. The
comparison tests could exercise the public conversion path instead.
For top-level Binary in Get mode, `std::str::from_utf8(bytes)` can also
supply a borrowed `&str` directly to the builder, avoiding the extra copy
through scratch. Invalid UTF-8 should retain safe/strict handling, shredding
should still reject binary as a string, and nested binary should use Arrow's
hex formatting.
##########
parquet-variant-compute/src/variant_to_arrow.rs:
##########
@@ -1204,16 +1302,61 @@ impl VariantPathRowBuilder<'_> {
}
}
+#[derive(Default)]
+pub(crate) struct TemporalFormats<'a> {
+ date: CompiledTimeFormat<'a>,
+ time: CompiledTimeFormat<'a>,
+ timestamp: CompiledTimeFormat<'a>,
+ timestamp_tz: CompiledTimeFormat<'a>,
+ null: &'a str,
+}
+
+impl<'a> TemporalFormats<'a> {
+ pub(crate) fn new(options: &CastOptions<'a>) -> Self {
+ let options = &options.format_options;
+ Self {
+ date: CompiledTimeFormat::new(options.date_format()),
+ time: CompiledTimeFormat::new(options.time_format()),
+ timestamp: CompiledTimeFormat::new(options.timestamp_format()),
+ timestamp_tz:
CompiledTimeFormat::new(options.timestamp_tz_format()),
+ null: options.null(),
+ }
+ }
+
+ pub(crate) fn date(&self) -> &CompiledTimeFormat<'a> {
+ &self.date
+ }
+
+ pub(crate) fn time(&self) -> &CompiledTimeFormat<'a> {
+ &self.time
+ }
+
+ pub(crate) fn timestamp(&self) -> &CompiledTimeFormat<'a> {
+ &self.timestamp
+ }
+
+ pub(crate) fn timestamp_tz(&self) -> &CompiledTimeFormat<'a> {
+ &self.timestamp_tz
+ }
+
+ pub(crate) fn null(&self) -> &str {
+ self.null
+ }
+}
+
+// Define a builder for converting variant values into a primitive Arrow array.
+// `, $shred` passes shredding state; `; temporal_formats: $name` binds the
temporal formats.
macro_rules! define_variant_to_primitive_builder {
(struct $name:ident<$lifetime:lifetime $(, $generic:ident: $bound:path )?>
|$array_param:ident $(, $field:ident: $field_type:ty)?| ->
$builder_name:ident $(< $array_type:ty >)? { $init_expr: expr },
- |$value: ident $(, $shred: ident)?| $value_transform:expr,
+ |$value: ident $(, $shred: ident)? $(; temporal_formats: $formats:ident)?|
$value_transform:expr,
Review Comment:
Can we remove the `temporal_formats` macro parameter and its generated
field, initialization, and local binding? No invocation uses it now that the
string builder is handwritten and owns its formats directly.
--
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]
