-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Support remaining functions in protobuf serialization, add expr_fn
for StructFunction
#8100
Conversation
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.
good finding.
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.
Thank you for the contribution @JacobOgle -- this is looking like a great start 🙏
I started the CI to run the tests on this PR and I left a suggestion
_ => Err(proto_error( | ||
"Protobuf deserialization error: Unsupported scalar function", | ||
)), |
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.
Can you please remove this catch all clause so the compiler will ensure all enum variants are covered?
_ => Err(proto_error( | |
"Protobuf deserialization error: Unsupported scalar function", | |
)), |
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.
Not a problem! I will start chipping away at the last two enum variants here in a bit. Thanks for the feedback and openness allowing the contribution! 🙏
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.
@alamb I was able to get the remaining cases covered with the exception to datafusion::ScalarFunction::StructFun
.
Browsing the code I am not seeing a supported function for this enum variant in expr_fn.rs
. The scalar_expr macro maps to enum variants in built_in_functions.rs
but has no match for StructFun. Should this be mapped to the Struct variant in the macro?
Apologies for the confusion, but appreciate any guidance.
macro_rules! scalar_expr {
($ENUM:ident, $FUNC:ident, $($arg:ident)*, $DOC:expr) => {
#[doc = $DOC ]
pub fn $FUNC($($arg: Expr),*) -> Expr {
Expr::ScalarFunction(ScalarFunction::new(
built_in_function::BuiltinScalarFunction::$ENUM,
vec![$($arg),*],
))
}
};
}
but only has
...
// struct functions
/// struct
Struct,
...
in the BuiltinScalarFunction::$ENUM
EDIT:
To follow up, if I add
scalar_expr!(Struct, struct_fun, val, "returns a vector of fields from the struct");
to expr_fn.rs
this passes the compiler checks and the tests. I just want to make sure this is the correct behavior before I commit.
expr_fn
for StructFunction
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.
Thank you @JacobOgle and @Syleechan -- this looks great -- a very nice improvement
Which issue does this PR close?
Closes #8098.
Rationale for this change
Suggested changes were made to catch most cases of datafusion::ScalarFunction
What changes are included in this PR?
Added the additional cases for
ScalarFunction::ArrowTypeof
,ScalarFunction::ToTimestamp
, 'ScalarFunction::Flatten'Are these changes tested?
Test suite seemed to pass.
Are there any user-facing changes?
Not entirely sure of this. My first commit to this project so I am still getting up to speed.
None that I am aware of. Same reasoning as above answer.