gianm commented on PR #15694: URL: https://github.com/apache/druid/pull/15694#issuecomment-1928739137
> Actually, thinking a bit more about it, maybe instead of my suggested pattern we should just focus on vectorizing the rest of the functions (since vector eval already has this pattern) and work on deprecating `eval` completely.... Are you suggesting forgetting about the optimization to re-use ExprEval for nonvectorized `eval`— like, not doing it at all— and instead focusing on allowing more queries to use the vectorized path? I suppose I'm good with that. The vector processor pattern already supports the kinds of things I mentioned here: https://github.com/apache/druid/pull/15694#issuecomment-1897600865 We'd need to up the priority of vectorizing `scan` and `topN` for this strategy to be a good one. With those engines not being vectorized, it means that even vectorizable exprs have to go through the nonvectorized eval path. I'm also down to do a "simple" improvement here, if there is one. It needs to be safe to use, though. It shouldn't give thread-unsafe behavior if someone accidentally forgets to call a method. @clintropolis suggestion of a type-safe way of doing that is best in general: type A represents the parsed expr, type B does evaluation of the expr, and there is a method on A that is used to get B. But if that's a big change it may make more sense to focus on vectorizing things, especially if the benefit of the change is relatively minor. @kgyrtkirk do you have benchmarks showing how big of an improvement the re-use of ConstantExpr would be? to help consider whether it's worth the effort / complexity of altering the Expr design to support re-use, or if we should put it aside to focus on vectorizing more things. -- 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]
