comphead commented on code in PR #7539: URL: https://github.com/apache/datafusion-comet/pull/7539#discussion_r4532981478
########## docs/source/contributor-guide/optimizing_expressions.md: ########## @@ -166,13 +170,19 @@ Cross-cutting principles behind the table: - **Watch capacity hints.** A `rows * rows` capacity hint (instead of `rows`) silently over-allocates; fix these when you find them. -## The no-regression rule +## Slower shapes need a justification -**Do not submit an optimization if any benchmark shape is meaningfully slower**, even if another -shape is dramatically faster. A change that is 90% faster with no nulls but 30% slower with dense -nulls is a trade-off, not a win, and should either be gated behind a runtime check that picks the -right path per batch or not submitted at all. A Comet optimization PR was closed for exactly this -reason (a 30% dense-null regression alongside a 90% no-null win). +Optimize for the common case. A shape may get slower when the common case gets faster and the PR +justifies it. For each shape that is meaningfully and reproducibly slower, the PR description gives +the measured cost, how often real workloads reach that shape and why, and the gain on the common +case. For example, an error path that fails the query runs at most once per partition, so a slower +error path can be acceptable. + +A shape that real workloads reach often is not an exception. A change that is 90% faster with no +nulls but 30% slower with dense nulls costs the many real columns that have dense nulls, so either +gate the fast path behind a runtime check that picks the right path per batch, or do not submit it. +A Comet optimization PR was closed for exactly this reason. A claim that a shape is rare needs +evidence about real workloads, not an assertion. Review Comment: Now that a merged optimization can make a shape slower, could "Record the performance audit" below ask for that too? Its template records the technique, the speedup, the PR and the benchmark, so the audit entry, which is what the next person tuning the expression reads, would not show that a shape was traded away or why. A clause such as "and any shape that got slower, with its cost" in the template, plus the same in the PR conventions' Testing line, would keep the justification next to the history. Two existing entries in `expression-audits/conversion_funcs.md` already say "no regression on any shape", so the line has room for it. -- 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]
