Collocate Decimal Array Validation Logic #2446
Conversation
|
Please add some description if this PR has been ready for review. Thank you. |
liukun4515
force-pushed
the
collation_decimal_array_code
branch
from
f0e2a45 to
3929723
Compare
tracked a issue to describe what the pr solves |
| .unwrap() | ||
| .validate_decimal_precision(precision), | ||
| other_width => { | ||
| panic!("invalid byte width {}", other_width); |
There was a problem hiding this comment.
Do you test that if this is a compile error or a runtime error?
There was a problem hiding this comment.
I don't test this error.
Guess this is a runtime error, when use BasicDecimalArray<OTHER_WIDTH> to call this method.
liukun4515
force-pushed
the
collation_decimal_array_code
branch
from
3929723 to
ed60bbc
Compare
| .as_any() | ||
| .downcast_ref::<Decimal256Array>() | ||
| .unwrap() | ||
| .validate_decimal_precision(precision), |
There was a problem hiding this comment.
I wonder if this logic should live in ArrayData, where all the rest of the validation logic lives?
There was a problem hiding this comment.
some usage of
arrow-rs/arrow/src/array/data.rs
Line 329 in 42e9531
arrow-rs/arrow/src/array/data.rs
Line 377 in 42e9531
arrow-rs/arrow/src/array/data.rs
Line 1037 in 42e9531
I will go through the usage, and check if it is necessary or not
There was a problem hiding this comment.
I'm suggesting that rather than this PR implementing additional validation, it should just use the validation logic that already exists in ArrayData?
There was a problem hiding this comment.
sounds great, maybe I can try it in the follow up pr.
we remain the original logic and just refactor this struct and impl now
I just concerned about the performance of the ArrayData. We have the benchmark which can be used to bench it easily.
@alamb add the validation for decimalarray with_precision_scale, I also have the same question. Why not we use the logic in the ArrayData?
There was a problem hiding this comment.
Consolidating validation in ArrayData sounds like a great plan to me. I can't remember any particular reason it isn't there
tustvold
changed the title
|
Benchmark runs are scheduled for baseline = 2185ce2 and contender = 0013170. 0013170 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2447
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?