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

Implement user defined planner for position #11243

Merged
merged 5 commits into from
Jul 6, 2024

Conversation

xinlifoobar
Copy link
Contributor

Which issue does this PR close?

Closes #11242

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added sql SQL Planner logical-expr Logical plan and expressions core Core DataFusion crate labels Jul 3, 2024
@xinlifoobar xinlifoobar closed this Jul 3, 2024
@xinlifoobar xinlifoobar reopened this Jul 3, 2024
@alamb
Copy link
Contributor

alamb commented Jul 3, 2024

🤔 We seem to be hitting the dreaded sql planner stack overflow in debug builds again

https://github.com/apache/datafusion/actions/runs/9778881627/job/26996930268?pr=11243

thread 'tokio-runtime-worker' has overflowed its stack
fatal runtime error: stack overflow
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`

Caused by:
  process didn't exit successfully: `/__w/datafusion/datafusion/target/debug/deps/sqllogictests-dd5da520160d5a7d` (signal: 6, SIGABRT: process abort signal)
Error: Process completed with exit code 101.

I think best way to fix this is to make a function (rather than inlining the code into the same big match expression)

#[derive(Default)]
pub struct PositionPlanner;

impl UserDefinedSQLPlanner for PositionPlanner {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayzhan211 do you suggest we have one planner for each module (like a UnicodePlanner rather than a PositionPlanner?

I think this is similar to what you are proposing with CoreFunctionsPlanner in #11273 (comment) in #11273

We could make this change in a follow on PR too

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jayzhan211 do you suggest we have one planner for each module (like a UnicodePlanner rather than a PositionPlanner?

Yes. And with different plan_* function

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #11305 to track. Thank you

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution @xinlifoobar -- this makes sense to me

Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm thanks @xinlifoobar but I have same concern as @alamb on having a separate planner

@alamb alamb merged commit 2af3d3a into apache:main Jul 6, 2024
23 checks passed
@alamb
Copy link
Contributor

alamb commented Jul 6, 2024

🚀 let's consolidate the planners as a follow on PR

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
* Implement user defined planner for position

* Fix format

* Move planner to session_state

* Extract function
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate logical-expr Logical plan and expressions sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement user defined planner for sql_position_to_expr
5 participants