peterxcli commented on PR #5771: URL: https://github.com/apache/datafusion-comet/pull/5771#issuecomment-5612275126
All three are fixed in 9b9817f8f. The density finding was correct and is the one that mattered. **`dense` was silently all-null.** Reproduced exactly: `common::is_null` derives `round(1 / ratio)`, which is 1 for any ratio above two thirds, so 0.875 gave 8192 nulls and zero valid rows. The stride is now taken over the *valid* rows above half, which yields the intended 7168 / 1024. Ratios at or below half are untouched, and no existing caller passes anything in `(0.5, 1.0)` -- `NULL_RATIOS` is 0.0, 0.1, 1.0 -- so no other benchmark's shapes move. `assert_density` now checks the realised null count against the requested ratio before timing, so a degenerate shape fails rather than being measured. Worth separating: the earlier -70% to -87% dense figures came from the old `extract_clock_fields.rs`, whose own selector was a genuine `i % 8 != 0`. The defect arrived with the consolidation, so it never produced a published number -- but the harness in the PR was wrong and you caught it before it did. **`assert_matches_arithmetic` was dropped in the fold.** Correct, and the PR body's claim was false for that harness; both are fixed. Every clock shape now gets a null-aware check -- null exactly where the input is null, values inside the field's range -- and shapes where no session offset applies additionally get an exact oracle recomputed from the generator rather than from arrow, so it validates the baseline implementation as well as this one. Dictionary and offset-session controls get the null-aware half, since no arithmetic oracle applies to them. The baseline run passes all 144 checks too. **Matched results after the repair.** 144 shapes, baseline captured on `bb9e74020` source, two head samples, absolute medians with 95% CI. 51 fast-path shapes have a baseline at or above 2 us and every one improves, by -82% to -94%: | shape | baseline us | head us | s1 | s2 | | --- | --- | --- | --- | --- | | `hour/utc_session/tz/8192/no_nulls` | 92.85 [89.72, 96.97] | 5.48 [5.47, 5.48] | -89.7% | -94.1% | | `minute/utc_session/tz/8192/no_nulls` | 113.71 [109.74, 117.73] | 7.13 [7.10, 7.16] | -89.2% | -93.5% | | `hour/la_session/ntz/524288/no_nulls` | 3140.50 [3105.00, 3177.90] | 291.92 [290.45, 293.62] | -89.2% | -90.6% | | `hour/utc_session/ntz/8192/no_nulls` | 55.15 [50.02, 63.27] | 5.46 [5.46, 5.47] | -82.4% | -89.6% | Thirteen shapes were slower in both samples. The repeat does not support any of them as cost: | shape | baseline us | head us (s2) | s1 | s2 | path | | --- | --- | --- | --- | --- | --- | | `minute/la_session/tz/8192/dense` | 20.97 [20.77, 21.33] | 21.13 [20.98, 21.31] | +88.1% | +0.6% | control | | `minute/la_session/tz/8192/no_nulls` | 175.28 [171.90, 180.71] | 173.82 [173.47, 174.20] | +77.1% | +0.5% | control | | `hour/la_session/dict1024/8192/no_nulls` | 6.52 [6.51, 6.54] | 6.61 [6.58, 6.66] | +37.0% | +1.3% | control | | `hour/la_session/tz/8192/all_null` | 0.89 [0.89, 0.89] | 0.96 [0.96, 0.97] | +2.0% | +7.9% | control | | `hour/la_session/ntz/524288/all_null` | 16.70 [16.68, 16.73] | 17.32 [17.16, 17.54] | +14.9% | +3.4% | fast | On the control comparison you asked for: the large sample-1 numbers on the `minute` and `second` `la_session` shapes are interference partway through that run. `hour/la_session` runs earlier in the same process and was stable at -5.3% / -4.7% across both samples on identical code, and the sample-2 absolutes land within a microsecond of the baseline -- `minute/la_session/tz/8192/no_nulls` is 175.28 -> 173.82 us, slightly *faster*. The largest movement that reproduces is `hour/la_session/tz/8192/all_null` at +2.0% / +7.9%, which is 0.89 -> 0.96 us; for an all-null batch both sides call `unary_opt` and visit zero valid indices, so there is no algorithmic difference there to explain a cost. You are also right that the sparse ratio moved from 12.5% to the shared 10% convention, so the tables above supersede the earlier ones entirely rather than extending them; the PR body now carries these numbers with the caveats inline. Still not established here, and not claimed: any query-level result, and anything on x86-64. -- 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]
