ming535 commented on code in PR #2694:
URL: https://github.com/apache/arrow-datafusion/pull/2694#discussion_r892297807
##########
datafusion/optimizer/src/limit_push_down.rs:
##########
@@ -43,61 +41,73 @@ impl LimitPushDown {
/// when traversing down related to "limit push down".
enum Ancestor {
/// Limit
- FromLimit,
- /// Offset
- FromOffset,
+ FromLimit {
+ skip: Option<usize>,
+ fetch: Option<usize>,
+ },
/// Other nodes that don't affect the adjustment of "Limit"
NotRelevant,
}
///
-/// When doing limit push down with "offset" and "limit" during traversal,
-/// the "limit" should be adjusted.
-/// limit_push_down is a recursive function that tracks three important
information
-/// to make the adjustment.
+/// When doing limit push down with "skip" and "fetch" during traversal,
+/// the "fetch" should be adjusted.
+/// "Ancestor" is pushed down the plan tree, so that the current node
+/// can adjust it's own "fetch".
///
-/// 1. ancestor: the kind of Ancestor.
-/// 2. ancestor_offset: ancestor's offset value
-/// 3. ancestor_limit: ancestor's limit value
+/// Ancestor's real "fetch" is extended with ancestor's "skip".
+/// If the current node is a Limit, then the adjusted ancestor "fetch" will
+/// replace the its own "fetch", and push down this "fetch" down the tree.
+/// ancestor's "skip" is always replaced by the current "skip" when the current
+/// node is a Limit.
///
-/// (ancestor_offset, ancestor_limit) is updated in the following cases
-/// 1. Ancestor_Limit(n1) -> .. -> Current_Limit(n2)
-/// When the ancestor is a "Limit" and the current node is a "Limit",
-/// it is updated to (None, min(n1, n2))).
-/// 2. Ancestor_Limit(n1) -> .. -> Current_Offset(m1)
-/// it is updated to (m1, n1 + m1).
-/// 3. Ancestor_Offset(m1) -> .. -> Current_Offset(m2)
-/// it is updated to (m2, None).
-/// 4. Ancestor_Offset(m1) -> .. -> Current_Limit(n1)
-/// it is updated to (None, n1). Note that this won't happen when we
-/// generate the plan from SQL, it can happen when we build the plan
-/// using LogicalPlanBuilder.
fn limit_push_down(
_optimizer: &LimitPushDown,
ancestor: Ancestor,
- ancestor_offset: Option<usize>,
- ancestor_limit: Option<usize>,
plan: &LogicalPlan,
_optimizer_config: &OptimizerConfig,
) -> Result<LogicalPlan> {
- match (plan, ancestor_limit) {
- (LogicalPlan::Limit(Limit { n, input }), ancestor_limit) => {
- let (new_ancestor_offset, new_ancestor_limit) = match ancestor {
- Ancestor::FromLimit | Ancestor::FromOffset => (
- None,
- Some(ancestor_limit.map_or(*n, |x| std::cmp::min(x, *n))),
- ),
- Ancestor::NotRelevant => (None, Some(*n)),
+ match (plan, ancestor) {
+ (
+ LogicalPlan::Limit(Limit {
+ skip: current_skip,
+ fetch: current_fetch,
+ input,
+ }),
+ ancestor,
+ ) => {
+ let new_current_fetch = match ancestor {
+ Ancestor::FromLimit {
+ skip: ancestor_skip,
+ fetch: ancestor_fetch,
+ } => {
+ if let Some(fetch) = current_fetch {
+ // extend ancestor's fetch
+ let ancestor_fetch =
Review Comment:
I think it is. I added some more comments in the code.
--
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]
