Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support PushDownLimit for LogicalPlan::Extension #12679

Closed
phillipleblanc opened this issue Sep 30, 2024 · 2 comments · Fixed by #12685
Closed

Support PushDownLimit for LogicalPlan::Extension #12679

phillipleblanc opened this issue Sep 30, 2024 · 2 comments · Fixed by #12685
Assignees
Labels
enhancement New feature or request

Comments

@phillipleblanc
Copy link
Contributor

phillipleblanc commented Sep 30, 2024

Is your feature request related to a problem or challenge?

I implemented a custom extension operator for DataFusion that logically wraps a TableScan, and the PushDownLimit optimizer rule doesn't support extension nodes (PushDownFilter does). This meant that my TableScans were always getting limit: None - which required pulling all of the data to then throw most of it away.

Describe the solution you'd like

I'd like PushDownLimit to be able to push down limits into Extension nodes as well.

I believe this will require changes to the UserDefinedLogicalNodeCore/UserDefinedLogicalNode traits to say if its safe to push the limit to the children or not. There may also need to be an API to say if the extension node itself supports limits?

Maybe something like:

/// Applies the limit to the extension node itself.
/// This should be false if the limit can push down to its inputs instead.
fn apply_limit(&self, limit: u64) -> bool {
        false
}

/// Indicates to the optimizer if its safe to push a limit down past
/// this extension node
fn apply_limit_to_inputs(&self) -> bool {
      false
}

I'm not convinced we need apply_limit, so maybe an incremental step would be to just add apply_limit_to_inputs?

Describe alternatives you've considered

I ended up rewriting my AnalyzerRule to an OptimizerRule to ensure the limit pushdown happened before the new extension node was added, but that feels more like a workaround.

Additional context

No response

@phillipleblanc phillipleblanc added the enhancement New feature or request label Sep 30, 2024
@austin362667
Copy link
Contributor

Thanks for your proposal @phillipleblanc . I believe it's a common use case, and it is indeed worth modifying the traits a bit.

@austin362667
Copy link
Contributor

take

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants