Blizzara commented on code in PR #712:
URL: https://github.com/apache/datafusion-comet/pull/712#discussion_r1689683289
##########
native/spark-expr/src/scalar_funcs/chr.rs:
##########
@@ -28,15 +28,7 @@ use arrow::{
use datafusion::logical_expr::{ColumnarValue, ScalarUDFImpl, Signature,
Volatility};
use datafusion_common::{cast::as_int64_array, exec_err, DataFusionError,
Result, ScalarValue};
-/// Returns the ASCII character having the binary equivalent to the input
expression.
-/// E.g., chr(65) = 'A'.
-/// Compatible with Apache Spark's Chr function
-pub fn spark_chr(args: &[ColumnarValue]) -> Result<ColumnarValue,
DataFusionError> {
- let chr_func = ChrFunc::default();
- chr_func.invoke(args)
-}
-
-pub fn chr(args: &[ArrayRef]) -> Result<ArrayRef> {
+fn chr(args: &[ArrayRef]) -> Result<ArrayRef> {
Review Comment:
these changes are not necessary so I can revert them if that's preferrable.
However given we already have the ScalarUDFImpl here, seems like a waste to not
use it (and also means the CometScalarUDF wraps a function that wraps a
ScalarUDFImpl, maybe it has some nanoseconds of perf impact)
--
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]
