tlfua commented on PR #7456: URL: https://github.com/apache/datafusion-comet/pull/7456#issuecomment-6759986168
@coderfender thanks for checking it so carefully. b948e2a05 addresses all four points: 1. You're right, `project` was doubling. `Masked` now builds its projection lazily, in a `OnceLock`, on the first `evaluate`, so building a nested wrapper no longer projects the level below it. `deeply_nested_short_circuits_reading_other_columns_are_planned_in_linear_time` in `null_short_circuit.rs` and `deeply_nested_lazy_operands_reading_other_columns_are_planned_in_linear_time` in `lazy_logical.rs` nest 64 levels that each read a different column, outer ones first, which would take 2^64 calls with the old code. 2. `Cast::cannot_fail` now accepts casts to `STRING` from integers, floating point numbers, booleans and decimals, after checking each against the native cast: `test_cannot_fail_casts_succeed_on_extreme_values_in_every_mode` casts the extreme values of each type in every eval mode. So `concat(name, CAST(id AS STRING))` is no longer wrapped. Dates and timestamps are left out on purpose: Arrow's formatter, which those casts use, can't format a value outside `chrono`'s range, which Spark can hold. Binary and nested types are left out too. Other casts that can't fail, such as widening a decimal or casting a boolean to or from a number, can be added later by the same rule. The benchmark has a `concat(q, CAST(b AS STRING))` shape now ([run](https://github.com/tlfua/datafusion-comet/actions/runs/42754962701), this head against the previous one on one runner). Unwrapped, it now costs the same as eager at every NULL ratio. One thing I didn't expect: wrapped, as on the previous head, it was faster at 50% NULL and above (1.14x at 50%, 1.47x at 70%, 2.07x at 90%: 111.7 vs 230.9 µs), because `concat` then skips formatting the rows whose result is NULL anyway. So not wrapping it gives up that speedup on mostly-NULL batches. I kept it unwrapped as you suggested, since it matches `main` and there is no error to guard against, but I'm happy to take the cast to string back out of `cannot_fail` if you'd rather keep the speedup. For `upper` and the other string functions I filed https://github.com/apache/datafusion-comet/issues/7587. 3. `NullShortCircuit`'s argument and `LazyLogical`'s right operand now share one `Masked` type with the projection, the lazy projected expression and `speculate`. Each caller only passes whether the operand is cheap, and the threshold is the same in both: speculate when at least half the rows need it. That changes `LazyLogical` at exactly half, from selecting the rows to speculating, which `half_the_rows_needing_an_argument_is_enough_to_speculate` covers. 4. `skippable` is now a required `Expression`. Fork CI passed on b948e2a05: https://github.com/tlfua/datafusion-comet/actions/runs/42754860727. The PR description has the updated table and test list. -- 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]
