scovich commented on code in PR #9454:
URL: https://github.com/apache/arrow-rs/pull/9454#discussion_r2867934391
##########
arrow-array/src/cast.rs:
##########
@@ -138,19 +138,19 @@ macro_rules! downcast_integer {
/// [`DataType`]: arrow_schema::DataType
#[macro_export]
macro_rules! downcast_integer_array {
- ($values:ident => $e:expr, $($p:pat $(if $pred:expr)* => $fallback:expr
$(,)*)*) => {
- $crate::downcast_integer_array!($values => {$e} $($p $(if $pred)* =>
$fallback)*)
+ ($values:ident => $e:expr, $($tail:tt)*) => {
Review Comment:
Right now we pay a combinatorial number of match arms (2**k). I wonder if it
would be clearer -- especially now that we have tt muncher arms -- to put the
canonical match arm first and then follow it with one tt muncher arm for each
of the `k` situations that needs fixup:
```rust
(($($values:ident),+) => $e:block $($p:pat $(if $pred:expr)? =>
$fallback:expr $(,)?)*) => {
$crate::downcast_integer!{
$($values.data_type()),+ =>
($crate::downcast_primitive_array_helper, $($values),+, $e),
$($p $(if $pred)? => $fallback,)*
}
};
// turn single value to value tuple
($value:ident => $($tail:tt)*) => {
$crate::downcast_integer_array!(($value) => $($tail)*)
};
// turn an expression into a block
(($($values:ident),+) => $e:expr, $($tail)*) => {
$crate::downcast_integer_array!(($($values),+) => {$e} $($tail)*)
};
}
```
I agree with your assessment that the `$($values:ident),+` block form --
which is inconsistent with the expression form -- is either an accident or a
hack to deal with a recursive rewrite, but it's technically a breaking change
to claw it back now. If we care, we just change the "turn single value..."
match arm to accept a list of values instead of only one.
Either way, the same simplification should work for
`downcast_temporal_array!` below.
##########
arrow-array/src/cast.rs:
##########
@@ -74,7 +74,7 @@ macro_rules! repeat_pat {
/// [`DataType`]: arrow_schema::DataType
#[macro_export]
macro_rules! downcast_integer {
- ($($data_type:expr),+ => ($m:path $(, $args:tt)*), $($p:pat $(if
$pred:expr)* => $fallback:expr $(,)*)*) => {
+ ($($data_type:expr),+ => ($m:path $(, $args:tt)*), $($p:pat $(if
$pred:expr)? => $fallback:expr $(,)*)*) => {
Review Comment:
If we're anyway fixing `*`...
```suggestion
($($data_type:expr),+ => ($m:path $(, $args:tt)*), $($p:pat $(if
$pred:expr)? => $fallback:expr $(,)?)*) => {
```
(several more below)
##########
arrow-array/src/cast.rs:
##########
@@ -546,19 +546,19 @@ macro_rules! downcast_dictionary_array_helper {
/// [`DataType`]: arrow_schema::DataType
#[macro_export]
macro_rules! downcast_dictionary_array {
Review Comment:
Does rust-analyzer also have trouble with this and `downcast_run_array!`
below?
Or is `downcast_primitive_array!` somehow special?
What about `downcast_temporal_array!`?
--
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]
