Re: [PR] Shift from Field to FieldRef for all user defined functions [datafusion]
alamb merged PR #16122: URL: https://github.com/apache/datafusion/pull/16122 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Shift from Field to FieldRef for all user defined functions [datafusion]
alamb commented on PR #16122: URL: https://github.com/apache/datafusion/pull/16122#issuecomment-2917166418 Thanks @timsaucer -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Shift from Field to FieldRef for all user defined functions [datafusion]
alamb commented on PR #16122: URL: https://github.com/apache/datafusion/pull/16122#issuecomment-2905409675 > Can you expand on this a little? Was there a specific metric you were watching to see performance improvements or is it looking at the code that we roughly have the same number of allocation operations? Or something else? It was the latter -- mostly I was thinking we'd be able to reuse `FieldRef` more than Field. There are some improvements for sure but for some reason I expected more. I don't think this is a big deal but wanted to mention it -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Shift from Field to FieldRef for all user defined functions [datafusion]
timsaucer commented on PR #16122: URL: https://github.com/apache/datafusion/pull/16122#issuecomment-2904018396 > I had hoped this would be a bigger improvement, but I think it at least sets us up for being more efficient / less String cloning going forward Can you expand on this a little? Was there a specific metric you were watching to see performance improvements or is it looking at the code that we roughly have the same number of allocation operations? Or something else? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] Shift from Field to FieldRef for all user defined functions [datafusion]
alamb commented on code in PR #16122:
URL: https://github.com/apache/datafusion/pull/16122#discussion_r2103200355
##
datafusion/functions/src/datetime/now.rs:
##
@@ -77,12 +77,13 @@ impl ScalarUDFImpl for NowFunc {
&self.signature
}
-fn return_field_from_args(&self, _args: ReturnFieldArgs) -> Result {
+fn return_field_from_args(&self, _args: ReturnFieldArgs) ->
Result {
Ok(Field::new(
Review Comment:
So in theory we could update this code to create the `FieldRef` once on
creation, and then return an `Arc::clone` rather than re-creating the Field
each time -- perhaps we can do that as some follow on PRs.
##
datafusion/expr/src/expr_fn.rs:
##
@@ -590,7 +591,7 @@ impl AggregateUDFImpl for SimpleAggregateUDF {
(self.accumulator)(acc_args)
}
-fn state_fields(&self, _args: StateFieldsArgs) -> Result> {
+fn state_fields(&self, _args: StateFieldsArgs) -> Result> {
Review Comment:
It is nice that this is now avoiding a deep copy of a bunch of `Field`s 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
