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

Register get_field by default #11959

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

leoyvens
Copy link
Contributor

This makes sense in principle, as all other core udfs are register by default in the context.

It also has a practical use, which is executing logical plans that have field access already de-sugared into get_field invocations.

Are these changes tested?

Yes

Are there any user-facing changes?

This is technically user-facing, though there aren't a lot of use cases for actually calling get_field.

This makes sense in principle, as all other core
udfs are register by default in the context.

It also has a practical use, which is executing
logical plans that have field access already de-sugared
into get_field invocations.
@github-actions github-actions bot added sqllogictest SQL Logic Tests (.slt) functions labels Aug 12, 2024
@jayzhan211
Copy link
Contributor

I think the reason that get_field is not enabled by default is that is it indirectly called by syntax. For example, struct()['a'] call get_field

GetFieldAccess::NamedStructField { name } => {
Ok(PlannerResult::Planned(get_field(expr, name)))
}

@leoyvens
Copy link
Contributor Author

That's right, this is not for end-users writing SQL queries. My use case is deserializing and executing logical plans that have s['a'] syntax desugared into get_field(s, 'a'), for that it is useful to have get_field registered in the context.

@alamb
Copy link
Contributor

alamb commented Aug 13, 2024

This came up in the ASF slack channel too: https://the-asf.slack.com/archives/C04RJ0C85UZ/p1723558402071579

Thus that is two users so I think this is a good idea to add

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 @leoyvens and @jayzhan211

This change makes sense to me given the usecase.

@@ -94,6 +94,7 @@ pub fn functions() -> Vec<Arc<ScalarUDF>> {
nvl2(),
arrow_typeof(),
named_struct(),
get_field(),
Copy link
Contributor

Choose a reason for hiding this comment

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

It might help to add the context of why this function is exported. Perhaps something like

Suggested change
get_field(),
// Note most users invoke `get_field` indirectly via field access syntax
// like `my_struct_col['field_name']` results in a call to `get_field(my_struct_col, "field_name")`
// It is also exposed directly for use cases such as serializing / deserializing plans with
// the field access desugared to calls to `get_field`
get_field(),

Copy link
Contributor

Choose a reason for hiding this comment

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

Added in follow on PR: #11996

@Dandandan Dandandan merged commit 482ef45 into apache:main Aug 14, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
functions sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants