You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "ivoson (via GitHub)" <gi...@apache.org> on 2024/05/11 03:08:29 UTC
[I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
ivoson opened a new issue, #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700
### Description
`Sig` is a mapping from spark expression to substrait name, used to convert spark functions to substrait and offload the funciton.
Currently, it includes below information:
```
case class Sig(expClass: Class[_], name: String)
```
All the registered functions are maintained in `ExpressionMappings` and serve for both velox and clickhouse backend. But some of them are supported by only one of the backend. For example: `Expm1` and `StringLocate` are only supported by clickhouse.
In the mapping, it's hard to tell which function is supported by which backend. Adding supported backend information here might be helpful for the maintenance/track the status of the functions.
--
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: commits-unsubscribe@gluten.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2106480847
> > Maybe, there are some cases that the expected fallback doesn't happen when validation fails. Do you have any findings that need us to fix?
>
> @PHILO-HE thanks for the response. I didn't find any cases need to be fixed. I created this issue because that I supposed the functions added in `ExpressionMappings` would be all supported by velox, but it was not. So it's not clear from the code to identify that how many of the expressions supported in velox/clickhouse. I am just wondering whether it's worth doing it. If not, we can rely on the docs up-to-date I think.
@ivoson, I think it is not worth doing that. cc @FelixYBW, @zhouyuan
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson closed issue #5700: Add supported backend information in Sig (signature for a expression)
URL: https://github.com/apache/incubator-gluten/issues/5700
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2111412864
@ivoson, some functions have multiple signatures. And it is possible that not all signatures are supported by native engine. If we add a fag in `ExpressionMappings`, we have to consider such partially supported state, which makes the code a bit complex. As I said, the native side validation can reduce the efforts for code maintenance, though it is not clear to developers to know which functions are not supported.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2109203576
> In our plan, the validation should all happens in native library. native library should have all the info it supports the expression/operators or not. But Velox itself doesn't have this feature due to its low priority, we still put all the logic in Gluten c++ now. It's where the mismatch comes from. It should be tracked when we nightly rebase to Velox but currently we didn't. How the sig() function can solve the mismatch issue
Thanks @PHILO-HE @FelixYBW for your explanation. The proposal is not targeting for plan validation, it's more like a metadata to point out the expression is supported by which backend.
For example, adding the metadata in Sig like `Sig(Add, 'add', Set(Velox, CH))` can tell that the expression is supported by both Velox and CH. There could be 2 scenarios we can leverage this:
1. When do plan validation, it can be eagerly detected if the function is not supported by the backend;
2. Track the function support status;
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2105503760
cc @PHILO-HE does this make sense? Please let me know if I missed anything. Thanks.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2109286962
> When do plan validation, it can be eagerly detected if the function is not supported by the backend;
We had this API https://github.com/apache/incubator-gluten/blob/182a0299993b38e04ee053158f71b2500c59eb09/gluten-core/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala#L556 to customize the supported expression list from backend implementations. Did you have a check on that code?
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2111427727
> @ivoson, some functions have multiple signatures. And it is possible that not all signatures are supported by native engine. If we add a fag in `ExpressionMappings`, we have to consider such partially supported state, which makes the code a bit complex. As I said, the native side validation can reduce the efforts for code maintenance, though it is not clear to developers to know which functions are not supported.
Understood. As the potential changes could introduce maintenance burden, closing the issue.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2109710994
> > When do plan validation, it can be eagerly detected if the function is not supported by the backend;
>
> We had this API
>
> https://github.com/apache/incubator-gluten/blob/182a0299993b38e04ee053158f71b2500c59eb09/gluten-core/src/main/scala/org/apache/gluten/backendsapi/SparkPlanExecApi.scala#L556
>
> to customize the supported expression list from backend implementations. Did you have a check on that code?
Thanks @zhztheplayer. I noticed this, but I am not very clear about what's is the design purpose of this API.
In the very beginning, I supposed that the expressions defined in `ExpressionMappings` should be supported by both engines(velox and ch), and the ones supported by only one of the engines should be defined with in above API. But it's not the case in current code implementation.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2105512493
Hi @ivoson, thanks for your proposal!
Actually, a kind of validate-by-trying-offloading (covering Java/Scala code & native code) is introduced in Gluten to reduce code maintenance efforts. At least for Velox backend (CH backend may still have some gaps to support validate-by-trying-offloading), if validation process fails (e.g., function signature is not found in velox), we expect fallback happens.
Maybe, there are some cases that the expected fallback doesn't happen when validation fails. Do you have any findings that need us to fix? Thanks!
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "ivoson (via GitHub)" <gi...@apache.org>.
ivoson commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2105548368
> Maybe, there are some cases that the expected fallback doesn't happen when validation fails. Do you have any findings that need us to fix?
@PHILO-HE thanks for the response. I didn't find any cases need to be fixed. I created this issue because that I supposed the functions added in `ExpressionMappings` would be all supported by velox, but it was not. So it's not clear from the code to identify that how many of the expressions supported in velox/clickhouse. I am just wondering whether it's worth doing it. If not, we can rely on the docs up-to-date I think.
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [I] Add supported backend information in Sig (signature for a expression) [incubator-gluten]
Posted by "FelixYBW (via GitHub)" <gi...@apache.org>.
FelixYBW commented on issue #5700:
URL: https://github.com/apache/incubator-gluten/issues/5700#issuecomment-2106549620
In our plan, the validation should all happens in native library. native library should have all the info it supports the expression/operators or not. But Velox itself doesn't have this feature due to its low priority, we still put all the logic in Gluten c++ now. It's where the mismatch comes from. It should be tracked when we nightly rebase to Velox but currently we didn't. How the sig() function can solve the mismatch issue
--
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: commits-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org