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

Port struct to datafusion-functions #9546

Merged
merged 7 commits into from
Mar 11, 2024
Merged

Port struct to datafusion-functions #9546

merged 7 commits into from
Mar 11, 2024

Conversation

yyy1000
Copy link
Contributor

@yyy1000 yyy1000 commented Mar 11, 2024

Which issue does this PR close?

Related #9285 .

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 physical-expr Physical Expressions labels Mar 11, 2024
Ok(Expr::ScalarFunction(ScalarFunction::new_udf(
Arc::new(ScalarUDF::new_from_impl(
datafusion_functions::core::r#struct::StructFunc::new(),
)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here it will use StructFunc so I add datafusion-functions as the dependency.

Copy link
Contributor

Choose a reason for hiding this comment

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

One of the goals of pulling datafusion functions out of the core is so that the built in functions aren't treated specially. Thus having the sql planner directly instantiate the function goes against that goal.

Can you please change this to call the struct() function by name (get_function_meta) instead of adding a dependency

For example
https://github.com/apache/arrow-datafusion/blob/4cd3c433004a7a6825643d6b3911db720efe5f76/datafusion/sql/src/expr/mod.rs#L173-L180

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed
learn something from it :)

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.

LGTM

@alamb
Copy link
Contributor

alamb commented Mar 11, 2024

Thank you @yyy1000 for helping along the 🚂 of function extraction

@alamb alamb changed the title Port Struct to datafusion-functions Port struct to datafusion-functions Mar 11, 2024
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 @yyy1000 and @jayzhan211 -- this is looking great

@alamb alamb merged commit 75ad221 into apache:main Mar 11, 2024
24 checks passed
@yyy1000 yyy1000 deleted the doc branch March 11, 2024 22:55
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 physical-expr Physical Expressions sql SQL Planner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants