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 create_struct & create_named_struct #11273

Merged
merged 11 commits into from
Jul 8, 2024

Conversation

dharanad
Copy link
Contributor

@dharanad dharanad commented Jul 4, 2024

Which issue does this PR close?

Closes #11221
Closes #11222

Rationale for this change

Part of #11207

What changes are included in this PR?

Are these changes tested?

Using existing test cases

cargo test

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 4, 2024
@dharanad dharanad marked this pull request as ready for review July 4, 2024 22:22
@dharanad dharanad changed the title Implement user defined planner for create_struct Implement user defined planner for create_struct & create_named_struct Jul 5, 2024
@dharanad dharanad requested a review from jayzhan211 July 5, 2024 08:06
@github-actions github-actions bot removed the core Core DataFusion crate label Jul 7, 2024
Comment on lines +654 to +659
for planner in self.planners.iter() {
match planner.plan_struct_literal(create_struct_args, is_named_struct)? {
PlannerResult::Planned(expr) => return Ok(expr),
PlannerResult::Original(args) => create_struct_args = args,
}
}
Copy link
Contributor Author

@dharanad dharanad Jul 7, 2024

Choose a reason for hiding this comment

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

Can we have one StructPlan that returns struct or named_struct based on the values?

let mut create_struct_args = if is_named_struct {
            self.create_named_struct_expr(values, schema, planner_context)?
        } else {
            self.create_struct_expr(values, schema, planner_context)?
        };

The logic here should be inside planner

@jayzhan211 Is this what u suggesting ?

If you were suggesting to move create_struct_expr & create_named_struct_expr to planner. I see an issues over there i.e these functions intnerally call self.sql_expr_to_logical_expr. I would rather have it here to keep it simple. Sorry if i understood it wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe like #11318 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on my understanding we also have a named_struct function which is an alias for struct function with input expressions optionally named.

In this PR i am not moving named_struct function to udp.

My understanding was to move create_named_struct & create_struct which were solely handling struct function.

Copy link
Contributor

@jayzhan211 jayzhan211 Jul 8, 2024

Choose a reason for hiding this comment

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

Can we have one StructPlan that returns struct or named_struct based on the values?

let mut create_struct_args = if is_named_struct {
            self.create_named_struct_expr(values, schema, planner_context)?
        } else {
            self.create_struct_expr(values, schema, planner_context)?
        };

The logic here should be inside planner

@jayzhan211 Is this what u suggesting ?

If you were suggesting to move create_struct_expr & create_named_struct_expr to planner. I see an issues over there i.e these functions intnerally call self.sql_expr_to_logical_expr. I would rather have it here to keep it simple. Sorry if i understood it wrong.

Right, they are not possible to move into the planner. Another thing we can improve on is, forgetting about create_struct_expr and use create_named_struct_expr and named_struct. But, we can change this in next PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Liked mentioned earlier. Thats a great idea, i will make the change in another PR. Thanks for your help @jayzhan211

Copy link
Contributor

@jayzhan211 jayzhan211 left a comment

Choose a reason for hiding this comment

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

👍

@jayzhan211
Copy link
Contributor

Thanks @dharanad and @alamb

@jayzhan211 jayzhan211 merged commit 1e39a85 into apache:main Jul 8, 2024
23 checks passed
@dharanad
Copy link
Contributor Author

dharanad commented Jul 8, 2024

Thank for your time and help @jayzhan211 & @alamb

@dharanad dharanad deleted the move-create_struct-to-udf branch July 8, 2024 07:46
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
…ruct` (apache#11273)

* add UserDefinedSQLPlanner for create struct

* fix linting

* add create name struct user defined sql planner

* simplify

* refactor

* refactor

* remove named_struct from functions

* formatting

* revert 953ad31

* update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 create_named_struct Implement user defined planner for create_struct
3 participants