[CALCITE-7680] PruneEmptyRules should prune Sort when OFFSET is greater than or equal to max input rows - #5132
[CALCITE-7680] PruneEmptyRules should prune Sort when OFFSET is greater than or equal to max input rows#5132xuzifu666 wants to merge 6 commits into
Conversation
| * <li>Sort[offset=5](input with at most 2 rows) becomes Empty | ||
| * </ul> | ||
| * | ||
| * <p>It relies on {@link org.apache.calcite.rel.metadata.RelMdMaxRowCount} |
There was a problem hiding this comment.
How the rule is implemented should not be documented here, but in the rule body.
There was a problem hiding this comment.
Thanks for the reminder; this part doesn't actually need a comment, and the existing rules already cover it, so I'll remove that comment for now.
| // no rows. RelMdMaxRowCount#getMaxRowCount(Sort) already subtracts the | ||
| // offset from the input row count, so the Sort is definitely empty. | ||
| return sort.offset instanceof RexLiteral | ||
| && RelMdUtil.isRelDefinitelyEmpty(call.getMetadataQuery(), sort); |
There was a problem hiding this comment.
If something is definitely empty, why does it matter if it's a sort or some other operation? Why does the offset matter?
There was a problem hiding this comment.
The rule targets Sort specifically because PruneEmptyRules traditionally separates structural pruning (child is empty Values) from metadata-based pruning. This rule fills the metadata-based gap for Sort: an OFFSET that skips all rows.
The offset check is there for clarity and to avoid invoking the metadata provider on Sorts without an OFFSET, where emptiness is already covered by SORT_INSTANCE or SORT_FETCH_ZERO_INSTANCE. Maybe we could consider unifying the three Sort rules into one metadata-based rule in the future.
There was a problem hiding this comment.
If you can merge two rules without increasing complexity you should.
There was a problem hiding this comment.
Yes, I merged SORT_FETCH_ZERO_INSTANCE and SORT_OFFSET_INSTANCE into a single SORT_EMPTY_INSTANCE rule that simply checks RelMdUtil.isRelDefinitelyEmpty. This covers LIMIT 0, OFFSET >= maxRowCount, and empty input with one rule and no extra complexity. SORT_FETCH_ZERO_INSTANCE is kept as a deprecated alias for backward compatibility. This impact should be minimal.
e20e2f5 to
a4276c8
Compare
| return sort.fetch != null | ||
| && !(sort.fetch instanceof RexDynamicParam) | ||
| && RexLiteral.bigDecimalValue(sort.fetch).equals(BigDecimal.ZERO); | ||
| final Sort sort = call.rel(0); |
There was a problem hiding this comment.
I think this is an improvement, but I still don't understand why this cannot be applied to any Rel node.
There was a problem hiding this comment.
You're right that isRelDefinitelyEmpty itself is generic.
The reason this rule is specific to Sort is that the replacement logic is not generic: RemoveEmptySingleRule can safely turn any SingleRel into an empty Values, but Join, Union, Intersect, Minus, and Aggregate each need their own logic when they are empty. This rule reuses RemoveEmptySingleRule because Sort is a SingleRel. A fully generic prune anything that is definitely empty rule would need a way to know how to rewrite each operator type correctly. The existing EMPTY_TABLE_INSTANCE follows the same pattern: it uses isRelDefinitelyEmpty but only for TableScan.
There was a problem hiding this comment.
What matters is that the operator has a single OUTPUT, and they all do.
Maybe you can find a way to do it, even if it means writing a bit more code to handle operators with more than 1 input.
There was a problem hiding this comment.
OK, I generalized ZeroMaxRowsRuleConfig (EMPTY_TABLE_INSTANCE) to match any RelNode that is definitely empty, not just TableScan. It excludes Values and TableModify (the latter may have side effects). This removes the need for a separate Sort-specific rule, so SORT_EMPTY_INSTANCE was deleted and SORT_FETCH_ZERO_INSTANCE is now a deprecated alias. The rule still relies on RelMdUtil.isRelDefinitelyEmpty, so the logic remains simple and covers LIMIT 0, large OFFSET, and any other case where Sort's max row count is zero.
|
mihaibudiu
left a comment
mihaibudiu
left a comment
There was a problem hiding this comment.
Please see @julianhyde's comment in Jira
Thanks for the reminder！I’ve outlined my views in Jira; please take a look when you have a moment to see if there are any issues. @mihaibudiu |
2 participants
jira: https://issues.apache.org/jira/browse/CALCITE-7680