-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Fix NULL["field"]
for expr_API
#10655
Conversation
NULL["field"]
for expr_API
@@ -106,6 +106,9 @@ impl ScalarUDFImpl for GetFieldFunc { | |||
}; | |||
let access_schema = GetFieldAccessSchema::NamedStructField { name: name.clone() }; | |||
let arg_dt = args[0].get_type(schema)?; | |||
if arg_dt.is_null() { | |||
return Ok(DataType::Null); | |||
} | |||
access_schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I quickly fixed it before noticing that there was already a PR
I think we can remove GetFieldAccessSchema
, get_accessed_field
. 6cb45b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jayzhan211
I agree the change in 6cb45b5 to inline things looks nice -- shall we make another PR?
Thank you for the speedy review @jayzhan211 |
Which issue does this PR close?
Closes #10654
Rationale for this change
This used to work (and we used it in IOx when we were programatically creating expressions):
Now it returns an error:
What changes are included in this PR?
Support
NULL["field"
Are these changes tested?
Yes, new unit tests are added
Are there any user-facing changes?
Fix regression