feat(array): cast bool and primitive values to Utf8 - #9261
Conversation
36c6f4f to
ff0d6f9
Compare
Signed-off-by: Huaijin <haohuaijin@gmail.com>
…r path Signed-off-by: Huaijin <haohuaijin@gmail.com>
Signed-off-by: Huaijin <haohuaijin@gmail.com>
Drop the TestResult alias and inline crate::Canonical qualifications in the primitive cast tests, move to_bit_buffer out of the all-null bool cast path, and cover f16 NaN/inf formatting in the array-path utf8 cast. Signed-off-by: Huaijin <haohuaijin@gmail.com>
db1e564 to
d12b54a
Compare
|
hi @AdamGS , do you have time take a look? |
Merging this PR will degrade performance by 2.12%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
There was a problem hiding this comment.
please use generics and a macro each macro (we use elsewhere)
There was a problem hiding this comment.
why did you need the new trait where did the other definitions come from where could we place this code?
There was a problem hiding this comment.
this is for unify the float to utf8 convert to use append_float_values_to_utf8 func
fn append_float_values_to_utf8<T>(builder: &mut VarBinViewBuilder, values: &[T], mask: &Mask)
where
T: FloatToUtf8,
{
let mut formatter = T::formatter();
append_values_to_utf8(builder, values, mask, |builder, value| {
builder.append_value(value.format(&mut formatter));
});
}i follower the arrow-rs, float32/float64 use ryu, float16 use display, https://github.com/apache/arrow-rs/blob/fc16607ccfdb298557ed6630742941761ac18ce3/arrow-cast/src/display.rs#L711-L734
|
Why did you add bool -> prim here too? |
Signed-off-by: Huaijin <haohuaijin@gmail.com>
|
Thanks for your reviews @joseph-isaacs , i apply you suggestion in 2c3825c.
sorry make you misunderstand, the pr #8621 impl the |
| let views = if true_count <= len - true_count { | ||
| let mut views = BufferMut::full(false_view, len); | ||
| values.for_each_set_index(|index| views[index] = true_view); | ||
| views | ||
| } else { | ||
| let mut views = BufferMut::full(true_view, len); | ||
| (!&values).for_each_set_index(|index| views[index] = false_view); | ||
| views | ||
| }; | ||
|
|
There was a problem hiding this comment.
can you explain the trick
There was a problem hiding this comment.
first init the full buffer with more frequent value(true/false), then only overwrite the another(false/true) positions, that minimize the index write.
|
This new a few comments explain the complex/interesting stuff and thoughts about where to put new trait or can we extend existing ones? |
|
please do split |
split it out of this pr |
Rationale for this change
DataFusion can push
CAST(... AS STRING)into Vortex scans, but boolean and primitive arrays currently lack native casts toUtf8, causing pushed queries to fail with a missingCastKernel.Related to #6211, #8621, and #6702.
What changes are included in this PR?
BoolArray -> Utf8andPrimitiveArray -> Utf8casts.Validation:
cargo nextest run -p vortex-arraycargo nextest run -p vortex-datafusionCAST(... AS STRING)with projection pushdown enabled, and verifies both values and nulls.developwith a missingBool -> Utf8cast kernel and passes with this PR.cargo clippy -p vortex-datafusion --all-targets --all-featurescargo +nightly fmt --allWhat APIs are changed? Are there any user-facing changes?
No public API signatures are changed.
Boolean and primitive values can now be cast to
Utf8inside Vortex, including DataFusion projection pushdown.