alamb commented on issue #22189: URL: https://github.com/apache/datafusion/issues/22189#issuecomment-4500642692
Update here * @xudong963 tried this in https://github.com/apache/datafusion/pull/22353 🏆 However, upon review I think it turns out we can't use lazy registration as that would change the semantics - https://github.com/apache/datafusion/pull/22353#pullrequestreview-4330375309 However, I do think we could reduce the overhead of creating metrics in general by avoiding the string allocations. I am copying my comments from https://github.com/apache/datafusion/pull/22353#discussion_r3275629426 here as I think they are relevant: > One challenge is that there are many many new strings with the file name. Maybe it is worth looking into if we could replace the owned string in Label with a Arc<str> (as another PR) > > https://github.com/apache/datafusion/blob/3aefba75be1ec571849136de5f2d575e6c505b7b/datafusion/physical-expr-common/src/metrics/mod.rs#L526-L527 > > It might be tricky however, as we would have to retain the special case for `&'static str` 🤔 > > Maybe we could make a new thing instead of Cow like ```rust enum ArcCow { /// Static string Static(&'static str), /// Shared arc Shared(Arc<str>), /// Owned allocation Owned(String) } ``` > With some finagling (like `impl Into<ArcCow>` I suspect we could make it mostly backwards compatible with the existing APIs / callsites 🤔 -- 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]
