andygrove commented on PR #5193: URL: https://github.com/apache/datafusion-comet/pull/5193#issuecomment-5192978796
Thanks for the rework. The scan idiom reads well, and the test additions cover the paths I was worried about. I checked out the head commit and ran things rather than reading the diff alone: - `cargo test -p datafusion-comet-spark-expr make_decimal`, 12 passed. `cargo test -p datafusion-comet columnar_to_row`, 20 passed, up from 18. - `cargo clippy -p datafusion-comet-spark-expr -p datafusion-comet --all-targets -- -D warnings` and `cargo fmt --all -- --check`, both clean. - **The null-slot garbage question is settled and it is fine.** `unary` widens every slot, so a null slot now carries arbitrary bits where the old `collect()` wrote zero. Since `UnsafeRow` hashes raw bytes, that could have been a real problem. It is not. `write_row_typed` sets the null bit and skips the value write, and the row buffer is zero-resized first, so the emitted bytes are unchanged. Your `test_array_null_slot_garbage_not_scanned` covers the expression side of the same question. - **Dropping `scale` from the validity check is safe.** In arrow, `validate_decimal_precision` only uses `scale` to format the error message, and the `precision > 38` case maps to "offender" under both functions. - **The `main` column of your table reproduces.** I ported the bench onto `apache/main` and measured 31.5 / 34.9 / 34.6 µs against your 32.38 / 35.84 / 35.44. My earlier 19 µs figure came from a different hand-rolled harness, so that was my mistake, not a problem with your numbers. Three things I would like to see before merge. **`with_precision_and_scale` does not rescan.** The comment above `with_data_type` gives that as the reason for choosing it, but in arrow 58.4.0 it is `validate_decimal_precision_and_scale` on the precision and scale pair followed by a struct update. It is O(1). The value scan lives in a separate `validate_decimal_precision` method that nothing calls here. Since it costs nothing, could you use it instead? `with_data_type` only asserts the DataType variant, so a bad precision would silently build an invalid array rather than erroring. It would also line this up with `checkoverflow.rs`, which uses `with_precision_and_scale` on all three of its branches, and with the `columnar_to_row.rs` half of this PR. **The bench has no overflowing shape, and the stated reason for that does not hold.** `benches/make_decimal.rs` says every i64 fits at precision 18, but `Decimal128(18)` maxes out just under 10^18 and `i64::MAX` is around 9.2×10^18. Your values top out at 9,999,900 so they do fit in practice. The gap that leaves is that the non-ANSI overflow branch now runs a full extra `null_if_overflow_precision` pass that `main` did not, and nothing measures it. Could you add a shape where some fraction of values exceed the target precision, in both ANSI and non-ANSI? That is the one path where this rewrite could plausibly lose ground. **The `unary` + scan column does not reproduce for me.** On the head commit I get 6.96 / 8.56 / 9.33 µs for the three non-ANSI shapes against 5.90 / 6.87 / 6.19 in the table, so dense nulls comes out at 3.7x rather than 5.72x. My `main` numbers land within 3% of yours, so this is not machine speed. I suspect the table predates the merge from `main`. Could you regenerate it on the current head? The change is a clear win either way. I would just like the numbers in the description to be ones a reader can reproduce. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
