apache
[VL] Respect prefix when register functions and convert function name
Description
Background
Gluten now use Velox codebase's presto function and implement spark specific version if it has same name but different behavior, and Velox already has prefix when register functions.
Propose
Use such prefix to distinguish presto function and spark function which has same name, it bring benefits
- when we want change few codes but hard to submit/wait Velox PR
- avoid invoke wrong implementation, current register codes require ensure invoke order.
- better readability.
Needed works:
- invoke registerFunction with prefix, such as
registerFunction("presto_")andregisterSparkFunction("spark_"). - do conversion work in native side, maintain a function name maps which map function name to target, such as [sum -> presto, avg -> spark], then convert substrait function name to mapped Velox function.
cc @zhouyuan @rui-mo
Here are my thoughts: https://github.com/oap-project/gluten/pull/2336#issuecomment-1774255145.
In the long term, I think we will only register spark sql functions after all the needed functions are supported. Currently, presto sql functions are registered just because the lack of functionality in spark sql functions.
I agree the future plan, but I believe it can not be achieved and no one would like implement duplicate functions from presto to spark, unless if your folks have this plan, and meta/velox will not accept too I think.
I just want know the concern about the idea of this propose, does this introduce any misleading idea? I believe choose accurate function always has benefits and help us to identify how many presto functions were used and spark used now.
To achieve that, we do not need to implement duplicate functions in presto and spark sql. It is a usual way in Velox to include a header from presto sql, and register it as a spark sql function if their semantics are the same.
I just want know the concern about the idea of this propose, does this introduce any misleading idea? I believe choose accurate function always has benefits and help us to identify how many presto functions were used and spark used now.
The proposal looks nice. I'm wondering how much this work is to distinguish function names, and if it's worth doing that considering the benefits it can bring. The work I can think of includes:
- Add correct prefix for all function names before passing to Velox. Currently, there's not a function list in Gluten native code, but only a mapping from Substrait function name to Velox function name if they are different. To add correct prefix for all functions, seems a full function list is needed.
- During function validation, firstly use spark prefix to compile. If there's error, use presto prefix to try again.
- Use correct function name in the code when referring to one function.
@marin-ma Maybe we could have a discussion here on how to finally resolve the registration issue you met. It seems below two options both require some extra work.
- Remove Presto functions' registration from Gluten.
- Use a prefix to register Presto or Spark functions.
@marin-ma Maybe we could have a discussion here on how to finally resolve the registration issue you met. It seems below two options both require some extra work.
- Remove Presto functions' registration from Gluten.
- Use a prefix to register Presto or Spark functions.
@rui-mo Thanks. I vote for adding prefix to Presto function. With this approach, we will need to establish and maintain a clear mapping to identify which functions still use presto implementation.
we will need to establish and maintain a clear mapping to identify which functions still use presto implementation.
@marin-ma That makes sense to me. After figuring out and establishing the full list of Presto functions, is the next step to register these functions under Spark in Velox? Possibly this work cannot be completed in a short time, but it seems finally we can remove the registration for Presto functions, and the function list is also not needed. How do you think?
we will need to establish and maintain a clear mapping to identify which functions still use presto implementation.
@marin-ma That makes sense to me. After figuring out and establishing the full list of Presto functions, is the next step to register these functions under Spark in Velox? Possibly this work cannot be completed in a short time, but it seems finally we can remove the registration for Presto functions, and the function list is also not needed. How do you think?
@rui-mo Do we need to register back to Velox Spark functions? If the presto implementation already aligns with Spark's, then the function mapping can provide such information.
do conversion work in native side, maintain a function name maps which map function name to target
Any reason to maintain another mapping? We already have one mapping file https://github.com/oap-project/gluten/blob/main/shims/common/src/main/scala/io/glutenproject/expression/ExpressionNames.scala.
The topic would be valid to me since one of the advantages is that we may be able to avoid overwriting during registration: https://github.com/oap-project/gluten/blob/f80408c9bfa36c51bd8bae2716a292389c03b572/cpp/velox/operators/functions/RegistrationAllFunctions.cc#L71-L74
Which brought some uncertainty to our code. (E.g., when an untested Spark function is newly added)
@zhztheplayer It is to solve an issue met in recent rebase. Currently a quick fix is added, and please check https://github.com/oap-project/velox/commit/e09bf6c24a109e0f50a886fb494b95ccddbafdd0. Glad to hear your opinions. Thanks.
I encounter the same issue, so any guide on how to solve it? Thanks. https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2024633762
@zhli1142015 The issue we met before was one is registered as vector function, and the other as simple, so overwrite does not work. But if the two are both simple functions, overwrite should happen.