You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "wForget (via GitHub)" <gi...@apache.org> on 2024/04/02 02:31:11 UTC
[I] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
wForget opened a new issue, #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248
### Backend
VL (Velox)
### Bug description
`array_size(null)` results inconsistent with vanilla spark.
test sql:
```
SELECT array_size(null)
```
native engine returns: `-1`
vanilla spark returns: `null`
### Spark version
None
### Spark configurations
_No response_
### System information
_No response_
### Relevant logs
_No response_
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2146959653
> I think before the wForget's PR ready in velox, at least we can fallback if `sparkLegacySizeOfNull` been set to true, right ?
@wForget, do you have any progress? I can take over it if you have no bandwidth. @gaoyangxiaozhu, let's wait two or three days.
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2031522040
This is an example to let a function struct cover different inputs. Similarly, you need to add an extra `initialize` & `call` method, then register it.
https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/DateTimeFunctions.h#L999
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2031556401
> This is an example to let a function struct cover different inputs. Similarly, you need to add an extra `initialize` & `call` method, then register it. https://github.com/facebookincubator/velox/blob/main/velox/functions/prestosql/DateTimeFunctions.h#L999
Thanks, I'm trying to do it that way, and will send a pr later.
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2159597059
> > I think before the wForget's PR ready in velox, at least we can fallback if `sparkLegacySizeOfNull` been set to true, right ?
>
> @wForget, do you have any progress? I can take over it if you have no bandwidth. @gaoyangxiaozhu, let's wait two or three days.
Sorry, I was interrupted by something else, please feel free to send PR.
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2030993310
I want to add a new `SizeExpressionTransformer` to convert `size` function result. If `legacySizeOfNull` is false, convert the result `-1` to `null`.
@PHILO-HE Is this feasible?
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2031496625
> Hi @wForget, thanks for bringing up this issue!
>
> Looks velox has a config to control the behavior. https://github.com/facebookincubator/velox/blob/main/velox/functions/sparksql/Size.cpp#L35
>
> I note Gluten sets it according to Spark's config to align with Spark's "Size" function. For "ArraySize" function, we expect it's always false. https://github.com/apache/incubator-gluten/blob/main/cpp/velox/compute/WholeStageResultIterator.cc#L482
>
> For performance consideration, it may be better to directly do some changes in velox's size function, e.g., add support for two args [here](https://github.com/facebookincubator/velox/blob/main/velox/functions/sparksql/Size.cpp#L27). The extra arg is `legacySizeOfNull` flag. If Velox finds it is specified, it will use this flag and dismiss the config setting. Then on Gluten side, `SizeExpressionTransformer` can check whether `legacySizeOfNull` is consistent with Spark conf. If not, pass the flag along with the input to Velox. Does this make sense?
@PHILO-HE Thanks for your guidance, this makes sense to me, I will try it.
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2030967147
Since spark replaces `array_size` with `size` and specifies `legacySizeOfNull` as `false`, the velox's `size` function respects `spark.sql.legacy.sizeOfNull` session conf.
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "PHILO-HE (via GitHub)" <gi...@apache.org>.
PHILO-HE commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2031165636
Hi @wForget, thanks for bringing up this issue!
Looks velox has a config to control the behavior. https://github.com/facebookincubator/velox/blob/main/velox/functions/sparksql/Size.cpp#L35
I note Gluten sets it according to Spark's config to align with Spark's "Size" function. For "ArraySize" function, we expect it's always false.
https://github.com/apache/incubator-gluten/blob/main/cpp/velox/compute/WholeStageResultIterator.cc#L482
For performance consideration, it may be better to directly do some changes in velox's size function, e.g., add support for two args [here](https://github.com/facebookincubator/velox/blob/main/velox/functions/sparksql/Size.cpp#L27). The extra arg is `legacySizeOfNull` flag. If Velox finds it is specified, it will use this flag and dismiss the config setting. Then on Gluten side, `SizeExpressionTransformer` can check whether `legacySizeOfNull` is consistent with Spark conf. If not, pass the flag along with the input to Velox. Does this make sense?
--
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] [VL] `array_size(null)` results inconsistent with vanilla spark [incubator-gluten]
Posted by "gaoyangxiaozhu (via GitHub)" <gi...@apache.org>.
gaoyangxiaozhu commented on issue #5248:
URL: https://github.com/apache/incubator-gluten/issues/5248#issuecomment-2146783014
I think before the wForget's PR ready in velox, at least we can fallback if `sparkLegacySizeOfNull` been set to true, right ? @PHILO-HE
--
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