[Variant] Align cast logic related to utf8 with arrow-cast kernel - #10114
[Variant] Align cast logic related to utf8 with arrow-cast kernel#10114klion26 wants to merge 6 commits into
Conversation
github-actions
Bot
added
arrow
ba2dcbd to
408a83f
Compare
|
Now that #10145 is fixed we can go back to this! |
408a83f to
34f46cd
Compare
34f46cd to
82fd9a1
Compare
|
Rebased on the latest main branch, there are too many changes, I need to have a self review, will make this ready when self review done. |
7a55774 to
d2e09e8
Compare
| use std::iter::zip; | ||
|
|
||
| #[test] | ||
| fn test_compatible_cast_logic_with_cast_kernel() { |
There was a problem hiding this comment.
Some cast logic was implemented directly here, add a test to cover it.
| ) { | ||
| Ok(Some(v)) => { | ||
| self.builder.append_value(v); | ||
| let $builder = &mut self.builder; |
There was a problem hiding this comment.
Add this because that variant_to_string return String now, but the builder.append_value receives a &str
971f155 to
9f05bca
Compare
# Conflicts: # parquet-testing
9f05bca to
829e8cf
Compare
|
|
||
| fn write_timestamp( | ||
| /// Writes a timestamp value to the output using the given representation. | ||
| pub fn write_timestamp( |
There was a problem hiding this comment.
im not sure about adding this to our public api; we could probably get away with leaving it public and just copying the code required into the variant crate? especially as they seem to use default format + either no timezone or the default utc timezone
There was a problem hiding this comment.
realized there was a typo here; i meant get away with leaving it private*
There was a problem hiding this comment.
Changed; I meant to make the logic aligned with arrow-cast, even if we change the logic in arrow-cast later, but we have a test to cover this, and the logic is simple; copied to parquet-variant crate.
| /// let expected_d4 = NaiveDate::from_ymd_opt(2026, 6, 10).unwrap(); | ||
| /// assert_eq!(expected_d4, d4); | ||
| /// ``` | ||
| pub fn parse_date(string: &str) -> Option<NaiveDate> { |
There was a problem hiding this comment.
is this a leftover change? cant seem to find where this now being used in the diff
There was a problem hiding this comment.
This case just wants to show the supported format of this function; it is not used in the current change.
There was a problem hiding this comment.
so it doesnt need to be made public?
There was a problem hiding this comment.
Ah, this belongs to a future pr, leftover when rebasing and splitting the original pr. changed.
| // boolean -> string | ||
| let boolean_array = BooleanArray::from(vec![Some(true), Some(false)]); | ||
| let cast_array = cast(&boolean_array, &DataType::Utf8).unwrap(); | ||
| let boolean_utf8_array = cast_array.as_any().downcast_ref::<StringArray>().unwrap(); |
There was a problem hiding this comment.
| let boolean_utf8_array = cast_array.as_any().downcast_ref::<StringArray>().unwrap(); | |
| let boolean_utf8_array = cast_array.as_string::<i32>(); |
can use the downcasters here to make it more succinct: https://docs.rs/arrow/latest/arrow/array/trait.AsArray.html
|
@Jefffrey Thanks for the review, I've addressed the comments, please take another look when you're free. |
|
|
||
| fn write_timestamp( | ||
| /// Writes a timestamp value to the output using the given representation. | ||
| pub fn write_timestamp( |
There was a problem hiding this comment.
realized there was a typo here; i meant get away with leaving it private*
| /// let expected_d4 = NaiveDate::from_ymd_opt(2026, 6, 10).unwrap(); | ||
| /// assert_eq!(expected_d4, d4); | ||
| /// ``` | ||
| pub fn parse_date(string: &str) -> Option<NaiveDate> { |
There was a problem hiding this comment.
so it doesnt need to be made public?
| cast(×tamp_nano_arrow_array, &DataType::Utf8).unwrap(); | ||
| let timestamp_nano_cast_utf8_array = timestamp_nano_arrow_cast_array | ||
| .as_any() | ||
| .downcast_ref::<StringArray>() |
There was a problem hiding this comment.
fyi can replace more usages of this downcast pattern with just as_string::<i32>() in more of these tests, etc.
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
should be good once ci is green
|
I'll take a look today |
| 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)), |
There was a problem hiding this comment.
arrow-cast's ArrayFormatter uses ryu for float
arrow-rs/arrow-cast/src/display.rs
Lines 712 to 724 in d5cd0da
the difference:
#[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);
}| } | ||
|
|
||
| // convert a variant to an owned string. | ||
| pub(crate) fn variant_to_string(variant: &Variant<'_, '_>) -> Option<String> { |
There was a problem hiding this comment.
We should add FormatOptions 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.
| let _ = write!( | ||
| ret_str, | ||
| ", {}", | ||
| variant_to_string(&item).unwrap_or_default() |
There was a problem hiding this 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 🤷 (separate issue maybe?)
regardless of correctness of the Object support above, variant-cast to String shouldn't lose data.
#[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);
}
|
@sdf-jkl Thank for the review, filed a new issue to track the Variant::Object in Variant::List case, I've update the code, and will run some benchmark to see if there are any performance regression in arrow-cast, will ping you for another review when ready. |
|
run benchmark cast_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing variant-cast-utf8 (4eb2a37) to f299971 (merge-base) diff Run configurationrun benchmark cast_kernelsBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench cast_kernels File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing variant-cast-utf8 (4eb2a37) to f299971 (merge-base) diff Run configurationrun benchmark cast_kernelsCPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
Sorry @klion26, I wasn’t clear. The What I meant could be tracked separately is the pre-existing inconsistency in |
|
thanks @klion26, I think we need to work on some benchmarks before proceeding.
I will file the tickets to track this. |
|
Added follow-up issues for the missing benchmark coverage discussed here: |
It's fine to merge #10982 into the current pr; I thought that you preferred to separate them. and thanks for the double confirmation. Will take a look at this tomorrow. |
I thought the existing benchmarks are not exhaustive enough and we should work on them first. Want to make sure we don't introduce regressions |
c96837a to
800ec28
Compare
|
For the benchmarks, maybe need to complete until next week, still some inner work to be done today and tomorrow. |
5 participants
Which issue does this PR close?
What changes are included in this PR?
arrow-rsAre these changes tested?
Are there any user-facing changes?