You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "zhli1142015 (via GitHub)" <gi...@apache.org> on 2024/03/27 07:43:35 UTC
[PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
zhli1142015 opened a new pull request, #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150
## What changes were proposed in this pull request?
Add UT for bitwise_not & bitwise_xor.
## How was this patch tested?
UT
--
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2026730602
> > > Is this some regression?
> >
> >
> > @zhli1142015, seems before this [commit](https://github.com/facebookincubator/velox/commit/9c7eb6c1732de4240b90ad0bea03ff221eb8e39a), the overwriting still requires signatures are logically equivalent. We are considering to separate the sparksql/prestosql functions (see mentioned discussion link), which can also fix other discovered conflict cases.
>
> For this change, I think I need to register bitwise_* functions with spark prefix in gluten separately to work around , is this ok to you or I should wait for the long term solution? Thanks.
@zhli1142015, it's ok to firstly use this workaround. 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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2022136315
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICEGJ-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
Could you open an issue for this pull request on Github Issues?
https://github.com/apache/incubator-gluten/issues
Then could you also rename ***commit message*** and ***pull request title*** in the following format?
[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}
See also:
* [Other pull requests](https://github.com/apache/incubator-gluten/pulls/)
--
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: [PR] [VL] Enable bitwise_and, bitwise_not, bitwise_or, bitwise_xor for tinyint & int [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 merged PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150
--
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2026460816
> Is this some regression?
@zhli1142015, seems before this [commit](https://github.com/facebookincubator/velox/commit/9c7eb6c1732de4240b90ad0bea03ff221eb8e39a), the overwriting still requires signatures are logically equivalent. We are considering to separate the sparksql/prestosql functions (see mentioned discussion link), which can also fix other discovered conflict cases.
--
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2024633762
Hello @FelixYBW , @PHILO-HE , @rui-mo ,
This pr failed with error `Validation failed due to exception caught at file:SubstraitToVeloxPlanValidator.cc line:804 function:validate, thrown from file:ExprCompiler.cpp line:426 function:compileRewrittenExpression, reason:Found incompatible return types for 'bitwise_not' (BIGINT vs. TINYINT) for input types (TINYINT)..` this is very wierd as the function with correct signatures are already added in Velox.
In Gluten we reigister presto fucntion first then spark functions. Is this because of the overwrite behavior from `registerStatefulVectorFunction`? But for simple functions they use vectors to store function signatures `registerFunctionInternal`. So for vector functions the last signaturewins, but for simple functions, the first wins.
This is too confusing, should we make them same behaviors, maybe set `overwrite` as false for all spark vector functions, and register spark fucntions first in gluten?
I saw there are several discussions and fixes for such issue, please suggest any guidence to handle such case.
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2026581717
> > Is this some regression?
>
> @zhli1142015, seems before this [commit](https://github.com/facebookincubator/velox/commit/9c7eb6c1732de4240b90ad0bea03ff221eb8e39a), the overwriting still requires signatures are logically equivalent. We are considering to separate the sparksql/prestosql functions (see mentioned discussion link), which can also fix other discovered conflict cases.
For this change, I think I need to register bitwise_* functions with spark prefix in gluten separately to work around , is this ok to you or I should wait for the long term solution?
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2024767221
@zhli1142015,
So Velox is somehow trying to use prestosql's `bitwise_not`, not the one you added after the below resolving.
https://github.com/facebookincubator/velox/blob/main/velox/expression/SimpleFunctionRegistry.cpp#L143
We always let presto function registered firstly, thus spark function registration can overwrite the previous one if they have same signature (including return type, see [link](https://github.com/facebookincubator/velox/blob/main/velox/expression/SimpleFunctionRegistry.cpp#L48)). So in your case, the overwriting does not happen.
We have a discussion for some other conflict cases at https://github.com/apache/incubator-gluten/discussions/4759 where two fix approaches are suggested.
--
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: [PR] [VL] add test for bitwise_not & bitwise_xor [incubator-gluten]
Posted by "zhli1142015 (via GitHub)" <gi...@apache.org>.
zhli1142015 commented on PR #5150:
URL: https://github.com/apache/incubator-gluten/pull/5150#issuecomment-2024931008
> @zhli1142015, So Velox is somehow trying to use prestosql's `bitwise_not`, not the one you added after the below resolving. https://github.com/facebookincubator/velox/blob/main/velox/expression/SimpleFunctionRegistry.cpp#L143
>
> We always let presto function registered firstly, thus spark function registration can overwrite the previous one if they have same signature (including return type, see [link](https://github.com/facebookincubator/velox/blob/main/velox/expression/SimpleFunctionRegistry.cpp#L48)). So in your case, the overwriting does not happen.
>
> We have a discussion for some other conflict cases at #4759 where two fix approaches are suggested.
Looks the overwrite happens only if the function signature exactly matches.
https://github.com/facebookincubator/velox/blame/0c86a0aedb90bfa966087def7551a40fc3571a01/velox/core/SimpleFunctionMetadata.h#L498-L515C4
Is this some regression? I test bitwise_and, looks it also failed with same error.
`Validation failed due to exception caught at file:SubstraitToVeloxPlanValidator.cc line:804 function:validate, thrown from file:ExprCompiler.cpp line:426 function:compileRewrittenExpression, reason:Found incompatible return types for 'bitwise_and' (BIGINT vs. TINYINT) for input types (TINYINT, TINYINT)..`
--
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