asolimando commented on PR #23975: URL: https://github.com/apache/datafusion/pull/23975#issuecomment-5204775572
Thanks @gabotechs for this PR, the q-error report is a useful addition that will help improving our supports for statistics! One issue I hit while trying it out: for plans containing a fetch, the reported q-error is not reproducible between runs, which affects the `--compare` mode. Running the same query (`SELECT * FROM t ORDER BY a LIMIT 10`) four times against unchanged code and unchanged data: > run 1 SortPreservingMergeExec estimate=Inexact(10) runtime=10 q-error=1.00x > run 1 SortExec(TopK) estimate=Inexact(10) runtime=90 q-error=9.00x > run 1 DataSourceExec estimate=Inexact(2000000) runtime=320000 q-error=6.25x > run 2 SortExec(TopK) estimate=Inexact(10) runtime=70 q-error=7.00x change=✅ 22.2% > run 3 SortExec(TopK) estimate=Inexact(10) runtime=80 q-error=8.00x change=-14.3% > run 4 SortExec(TopK) estimate=Inexact(10) runtime=80 q-error=8.00x change=0.0% An earlier set of four runs on the same machine produced -50.0% and -166.7%, so the tool reports both improvements and regressions where nothing changed (it failed AA testing, basically). In my example query, the estimate column is stable at `Inexact(10)`, only the runtime column moves. The cause seems to be that that `SortExec` has `preserve_partitioning=[true]` and `fetch=10`, where each partition that gets fully consumed contributes up to 10 rows to `MetricsSet::output_rows`, which sums across partitions, while the estimate is capped at a single fetch (which is expected). How many partitions get drained before the `SortPreservingMergeExec` is satisfied, depends on scheduling, and it is not stable across runs. Some options to address the problem: 1. Exclude problematic operators for now (I think the benchmark still brings lots of values even with partial support, we need to document the limitations and we can improve later) 2. Change the way we report per-partition runtime rows for operators with `preserve_partitioning`, so both sides of the comparison use the same unit (I didn't really thought of all implications for this tbh) 3. Run each query multiple times and report the median (not a huge fan of this, but if this proves to be enough for now, we can always go for 2. or better alternatives later) Claude helped creating the attached repro script, [qerr_repro.sh](https://github.com/user-attachments/files/30788540/qerr_repro.sh), which runs from the root of a datafusion checkout, builds dfbench itself, and generates its own dataset. It covers just the query shared above, as that's a minimal reproducer for the only problem that could be detected using the existing benchmark. Note the spread depends on core count and load, so your numbers will differ from mine; the claim is that the runtime column varies, not the specific values. I suggest to use `RUNS=8` if the first four runs happen to agree, I couldn't test on more machines. -- 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]
