You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tshauck (via GitHub)" <gi...@apache.org> on 2024/05/18 23:11:32 UTC
[PR] feat: add hex scalar function [datafusion-comet]
tshauck opened a new pull request, #449:
URL: https://github.com/apache/datafusion-comet/pull/449
## Which issue does this PR close?
Related to https://github.com/apache/datafusion-comet/issues/341.
## Rationale for this change
I recently added `unhex` so this PR adds `hex`. It's another scalar function that isn't yet implemented in Comet.
I decided to do a native version here because datafusion has an `to_hex`, but it has different functionality. E.g. `to_hex(-1)` returns `ffffffffffffffff` vs `hex(-1)` returning `FFFFFFFFFFFFFFFF`. Spark `hex` also supports strings, but datafusion and postgres do not. I'm happy to start the discussion if it seems it could be merged upstream.
## What changes are included in this PR?
I added the `hex` scalar function.
## How are these changes tested?
Yes, I've added tests to the rust side as well as spark sql based tests in scala.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1615380269
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Sounds like QueryPlanSerde is failing the case match.
Wondering if you already have tried `"spark.sql.optimizer.excludedRules" -> "org.apache.spark.sql.catalyst.optimizer.ConstantFolding"` ?
Also right now, since the scalar code was removed from rust, it will fail if the native code happen to find scalar values...
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624513049
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
@viirya Am I correct in understanding that this PR is functionally correct but just not as efficient as possible? Perhaps we could consider having a follow-up issue to optimize this to rewrite the dictionary? It seems that we don't have a full example of dictionary rewrite for contributors to follow.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617839444
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
BTW now I remember `unpack_dictionary_type()` unpacks early for primitive types, so like the one for Int64 can be omitted.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1619376557
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
For this case, we can call
```
dictionary.with_values(Arc::new(values));
```
to construct a new dictionary array with new values.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608714060
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
Review Comment:
I remember unsigned ints currently do not work in other places https://github.com/apache/datafusion-comet/blob/main/spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala#L97
I think it is ok to leave it as TODO
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
+ // checkSparkAnswerAndOperator("SELECT hex(_9), hex(_10), hex(_11), hex(_12) FROM tbl")
+ checkSparkAnswerAndOperator("SELECT hex(_11), hex(_12) FROM tbl")
+
+ // strings, binary
+ checkSparkAnswerAndOperator("SELECT hex(_8), hex(_14) FROM tbl")
Review Comment:
What about other types like _1, _6, _7, _13, _15+?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613906351
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Let's also add scalar tests too
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614116423
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
I'm realizing that I'm not going to have time next week and didn't expect this PR to take this long, so I've removed the scalar handling for now and hopefully can follow an example in the future. https://github.com/apache/datafusion-comet/pull/449/commits/88bdcde1babcd75cb52e18164e1c945bbbbbf857
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617533349
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
Not only in Comet. I do remember in DataFusion not all functions/expressions support dictionary types. I suspect if there is a systematic approach to deal with it, because I think there is no general approach to process dictionary-encoded inputs for different functions/expressions. For example, some functions can directly work on dictionary values and re-create a new dictionary with updated values, but for some functions, it is impossible so it needs to unpack dictionary first.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608584669
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Thanks for the feedback. I updated the tests in https://github.com/apache/datafusion-comet/pull/449/commits/2bfcf25a6705ec47d857670c65dbba979ca10e23 to use `makeParquetFileAllTypes`. And with a couple of changes its tests passes w/ the ints, uint32 and uint64[1], and columns _8 and _14 which I think are string/binary types.
When the #451 goes in, I'll look at updating the tests to add it too.
[1]: I'm a bit confused because it looks like every int crossing into rust does so as an i64, which means rather than properly return say `FF` for u8, I'm returning `FFFFFFFFFFFFFFFF`. I need to look into this further.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613963307
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
We have several scalar tests i.e. `ceil and floor`, `scalar decimal arithmetic operations`, etc. I agree that the existing coverage is not great.
Since you have the code to handle scalar, it is best to test them. Or it is also okay to disable scalar for now and put it as TODO.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608699758
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
@advancedxy got a good point. Spark may be casting ints to `i64`. Then we only need to handle `i64` in Rust as long as it passes the tests.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624630592
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
@andygrove Yes. It's not efficient but should be correct.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1607703859
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
Review Comment:
By the way, there's `hex_encode` in `scalar_func.rs`, maybe we should moved that into this hex.rs instead, so related methods are grouped together.
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Spark hex's input types are:
```scala
override def inputTypes: Seq[AbstractDataType] =
Seq(TypeCollection(LongType, BinaryType, StringType))
```
I think we need to test input with these types. By the way, it would be helpful to test with randomized input, I think you can leverage the `generateRows` in #451 and test hex in one sql such as:
```
select hex(long_col) , hex(binary_col), hex(string_col) from $test_table
```
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
Review Comment:
hmm, I think it should be `:02X` instead?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608872166
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
+ // checkSparkAnswerAndOperator("SELECT hex(_9), hex(_10), hex(_11), hex(_12) FROM tbl")
+ checkSparkAnswerAndOperator("SELECT hex(_11), hex(_12) FROM tbl")
+
+ // strings, binary
+ checkSparkAnswerAndOperator("SELECT hex(_8), hex(_14) FROM tbl")
Review Comment:
1caa6fd adds the other columns... `_13` also fails failure.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1609228014
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
+ // checkSparkAnswerAndOperator("SELECT hex(_9), hex(_10), hex(_11), hex(_12) FROM tbl")
+ checkSparkAnswerAndOperator("SELECT hex(_11), hex(_12) FROM tbl")
+
+ // strings, binary
+ checkSparkAnswerAndOperator("SELECT hex(_8), hex(_14) FROM tbl")
Review Comment:
I updated dictionary handling, so I've added _13 back in. I think only the smaller uints are omitted now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608584669
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Thanks for the feedback. I update the tests in https://github.com/apache/datafusion-comet/pull/449/commits/2bfcf25a6705ec47d857670c65dbba979ca10e23 to use `makeParquetFileAllTypes`. And with a couple of changes its tests passes w/ the ints, uint32 and uint64[1], and the _8 and _14 which I think are string/binary types.
When the #451 goes in, I'll look at updating the tests to add it too.
[1]: I'm a bit confused because it looks like every int crossing into rust does so as an i64, which means rather than properly return say `FF` for u8, I'm returning `FFFFFFFFFFFFFFFF`. I need to look into this further.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1607218060
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
probably `makeParquetFileAllTypes` is a better example
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617547871
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
Review Comment:
style:
```suggestion
}
test("hex") {
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613965861
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Thanks, I agree it's best to test since they're there. I just genuinely thought it wasn't supported given the test coverage, etc. I'll have a look at adding some tests...
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614116423
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
I'm realizing that I'm not going to have time next week and didn't expect this PR to take this long, so I've removed the scalar handling for now and hopefully can follow and example in the future. https://github.com/apache/datafusion-comet/pull/449/commits/88bdcde1babcd75cb52e18164e1c945bbbbbf857
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1619578548
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
@viirya, thanks, where I'm a bit unclear is how to have the function return type also be a dictionary. The data type for the hex expression seems to be a Utf8, so I get `org.apache.comet.CometNativeException: Arrow error: Invalid argument error: column types must match schema types, expected Utf8 but found Dictionary(Int32, Utf8)` after making the update to use `with_values`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1619370071
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
We have the same issue currently in cast from string to other types.
@viirya Do we have an example somewhere of converting a dictionary array without unpacking?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624657434
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,306 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_largestring_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::LargeUtf8 => {
+ let array = as_largestring_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
+ Ok(ColumnarValue::Array(Arc::new(string_array_values)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Utf8) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_string_array(dict.values());
+ let values: Vec<Option<String>> = hexed_values
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let keys = dict.keys().clone();
+
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
+ Ok(ColumnarValue::Array(Arc::new(string_array_values)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Binary) => {
Review Comment:
This part of dictionary arrays handling can be rewritten to reduce duplicate actually. It can be follow-up though.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608872166
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
+ // checkSparkAnswerAndOperator("SELECT hex(_9), hex(_10), hex(_11), hex(_12) FROM tbl")
+ checkSparkAnswerAndOperator("SELECT hex(_11), hex(_12) FROM tbl")
+
+ // strings, binary
+ checkSparkAnswerAndOperator("SELECT hex(_8), hex(_14) FROM tbl")
Review Comment:
1caa6fd adds the other columns... `_13` also fails as the rust code is provide with a `Dictionary(Int32, Utf8)` that isn't matched on.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613950678
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Thanks for bringing this up. I guess I thought scalars weren't supported yet (maybe for UDFs only)? From what I could tell none of the other tests test scalars, i.e. most of them insert into a table, then query the table. E.g.,
```scala
test("Chr") {
Seq(false, true).foreach { dictionary =>
withSQLConf(
"parquet.enable.dictionary" -> dictionary.toString,
CometConf.COMET_CAST_ALLOW_INCOMPATIBLE.key -> "true") {
val table = "test"
withTable(table) {
sql(s"create table $table(col varchar(20)) using parquet")
sql(
s"insert into $table values('65'), ('66'), ('67'), ('68'), ('65'), ('66'), ('67'), ('68')")
checkSparkAnswerAndOperator(s"SELECT chr(col) FROM $table")
}
}
}
}
```
Also running scalar tests seems to fail for the other UDFs I spot checked, e.g.,
```
- trim *** FAILED *** (7 seconds, 507 milliseconds)
Expected only Comet native operators, but found Project.
plan: *(1) Project [HI AS upper(trim(hi))#125]
+- *(1) Scan OneRowRelation[] (CometTestBase.scala:186)
```
Should they be working w/ UDFs, and it's just nothing else tests for them?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#issuecomment-2130054380
I think I made the updates requested in the latest round. I left the dictionary handling the same, but I can look into flattening the dictionary specific to hex if you guys think it's a good idea. 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1615377307
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
> Do you have any concrete examples of how this works by any chance?
Hmm, it's been a while, I cannot find right away...
BTW my comment above is not a blocker. Since this PR already implemented it, we can follow up separately
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617541513
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
Review Comment:
`as_string_array` only works for `DataType::Utf8`.
`DataType::LargeUtf8` needs to use `as_largestring_array`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608635628
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
Review Comment:
I think I can move sha2/md5 related to scalar_funcs in a follow-up pr. It’s indeed a better place. For the hex_encode, maybe we need to extract it to hex_utils or simply in hex.rs, and make that crate public. I don’t think it’s a blocker to this PR. Just saying it so that it can be addressed 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#issuecomment-2123824670
@kazuyukitanimura @advancedxy ... re-requesting reviews from you two, please. I've updated the code to support dictionaries and removed some of the finer int types. Barring something happening on intel mac, the tests look to pass here: https://github.com/tshauck/arrow-datafusion-comet/actions/runs/9183637020.... 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1619578722
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
Review Comment:
[e7062c6](https://github.com/apache/datafusion-comet/pull/449/commits/e7062c61a42ec254ec5db203845ec66e21a56bf9)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614977230
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
I initially tried and got test failures because the native function isn't being recognized w/ scalars. The same issue I mentioned w/ `trim` above.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617538649
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
Review Comment:
Spark `Hex` expression only supports LongType, BinaryType, StringType.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617545329
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
Hmm, why we don't re-create a new dictionary array of string? `Hex` is a kind of function that doesn't change dictionary-encoded one. You can simply take new values and existing keys to create a new dictionary array.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617545329
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
Hmm, why we don't re-create a new dictionary array of string? `Hex` is a kind of function that doesn't change dictionary-encoded mapping. You can simply take new values and existing keys to create a new dictionary array.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1607158744
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Let's add more types and values, something like `makeRawTimeParquetFile`
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
Review Comment:
Wondering if we need to support more types?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1607197818
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Thanks, will have a look at `makeRawTimeParquetFile` and follow up.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1619622117
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
Review Comment:
129f00ab1379f16ac6f3a68bb1dc73dc1fcebeb8
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608871374
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Yeah, I think that's the case. I'll pare down the rust 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608872166
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,26 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // ints
+ checkSparkAnswerAndOperator("SELECT hex(_2), hex(_3), hex(_4), hex(_5) FROM tbl")
+
+ // uints, uint8 and uint16 not working yet
+ // checkSparkAnswerAndOperator("SELECT hex(_9), hex(_10), hex(_11), hex(_12) FROM tbl")
+ checkSparkAnswerAndOperator("SELECT hex(_11), hex(_12) FROM tbl")
+
+ // strings, binary
+ checkSparkAnswerAndOperator("SELECT hex(_8), hex(_14) FROM tbl")
Review Comment:
1caa6fd adds the other columns... `_13` also fails failure as the rust code is provide with a `Dictionary(Int32, Utf8)` that isn't matched on.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614516643
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
> The trick we used to use is adding Cast(child) in QueryPlanSerde that unpacks dictionaries.
Do you have any concrete examples of how this works by any chance? I remember I saw some ~~unnecessary~~ cast operation in the query plan serde file, didn't realize it was for unpacking dictionaries.
> We should consolidate the unpacking logic, otherwise we will need to add it every function. Or until that happens we can workaround with Cast
Yes, maybe this logic should added in the rust planner side, which can unpack the dictionary automatically if it knows the expression cannot handle dictionary types.
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
I think we can keep the scalar functions, and it should be pretty straightforward to test scalar input?
Namely, it should be something like:
```
select hex(10), hex('abc')
```
The constant literal should be encoded as a ScalarValue.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1615396200
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Thanks, yes, I did also try disabling ConstantFolding to no avail.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624649245
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
`hex(literal)` should be evaluated to literal early in Spark optimizer. So it won't hit the native `hex`. I wonder what test failure you saw?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617545329
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,296 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
Review Comment:
Hmm, why we don't re-create a new dictionary array of string?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1617547465
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
+ }
+ }
+ }
+ }
test("unhex") {
Review Comment:
style:
```suggestion
}
test("unhex") {
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1612495192
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
I also ran into issues with this in handling cast operations and ended up flattening the dictionary type first, but just for cast expressions. I agree that we need to look at this and see if there is a more systematic approach we can use.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608617934
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
Review Comment:
I'll look into `:02X`, but it seems to pass tests w/ `:01X`. Also on `hex_encode`, I wonder if the hash functions should go in `scalar_funcs/hash.rs` or something, then `hex_encode` could go there? Personal preference, but I think it'd be nice if `scalar_funcs` was refactored so it just had `create_comet_physical_fun` and the individual function impls went in `scalar_funcs`.
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,191 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::array::as_string_array;
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:01X}", byte)?;
Review Comment:
I'll look into `:02X`, but it seems to pass tests w/ `:01X`. Also on `hex_encode`, I wonder if the hash functions should go in `scalar_funcs/hash.rs` or something, then `hex_encode` could go there? Personal preference, but I think it'd be nice if `scalar_funcs` was refactored so it just had `create_comet_physical_fun` and the individual function impls went in `scalar_funcs/`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1608584669
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,46 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ val str_table = "string_hex_table"
+ withTable(str_table) {
Review Comment:
Thanks for the feedback. I update the tests in https://github.com/apache/datafusion-comet/pull/449/commits/2bfcf25a6705ec47d857670c65dbba979ca10e23 to use `makeParquetFileAllTypes`. And with a couple of changes its tests passes w/ the ints, uint32 and uint64[1], and columns _8 and _14 which I think are string/binary types.
When the #451 goes in, I'll look at updating the tests to add it too.
[1]: I'm a bit confused because it looks like every int crossing into rust does so as an i64, which means rather than properly return say `FF` for u8, I'm returning `FFFFFFFFFFFFFFFF`. I need to look into this further.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "advancedxy (via GitHub)" <gi...@apache.org>.
advancedxy commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1610001541
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
Review Comment:
Hmm. I don't think we should double the size for string here. The bytes are already hexed bytes, aren't they?
And by the way, I think hex_bytes and hex_bytes_to_string should be combined into a single function, something like:
```rust
fn hex_bytes(bytes: &[u8]) -> String {
let length = bytes.len();
let mut hex_string = String::with_capacity(bytes.len() * 2);
let mut i = 0;
while i < length {
write!(&mut hex_string, "{:X}", (bytes[i] & 0xF0) >> 4).unwrap();
write!(&mut hex_string, "{:X}", bytes[i] & 0x0F).unwrap();
// or simply write
// write!(&mut hex_string, "{:02X}", bytes[i]).unwrap();
i += 1;
}
hex_string
}
```
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
Review Comment:
is this needed?
If the num is a negative i64, there's a leading 1 in its binary representation. So, there's no need to add width into the format?
Please correct me if I'm wrong here, we can simply write `format!("{:X}", num)` here?
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
Not totally related to this PR. But this seems a bit unexpected. I believe many other codes in scalar_funcs and datafusion doesn't handle the dictionary type specially, such as `spark_ceil`, `spark_rpad` and `spark_murmur3_hash` or other functions registered in DataFusion. If we are going to handle dictionary types, we should also update these functions too? Or we should do it in a more systematic way, such as flatten the dictionary types first.
cc @sunchao @andygrove and @viirya for more inputs.
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
Review Comment:
I think it can be simplified to something like below, other places apply to this as well. Assuming `hex_string` already returns a string.
```suggestion
let hexed_array: StringArray = array
.iter()
.map(|v| v.map(|v| hex_string(v))
.collect();
```
you can refer src/execution/datafusion/expressions/scalar_funcs.rs:685 for similar 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1614977230
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
I tried that already and got test failures because the native function isn't being recognized w/ scalars. The same output I mentioned w/ `trim` above.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613948812
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
The trick we used to use is adding `Cast(child)` in `QueryPlanSerde` that unpacks dictionaries.
We should consolidate the unpacking logic, otherwise we will need to add it every function. Or until that happens we can workaround with `Cast`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "tshauck (via GitHub)" <gi...@apache.org>.
tshauck commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1613965861
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
Thanks, I agree it's best to test since they're there. I just genuinely thought it wasn't supported given the test coverage, the errors on the other functions, etc. I'll have a look at adding some tests...
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1615377307
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,371 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError, ScalarValue,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ if num >= 0 {
+ format!("{:X}", num)
+ } else {
+ format!("{:016X}", num as u64)
+ }
+}
+
+fn hex_bytes(bytes: &[u8]) -> Vec<u8> {
+ let length = bytes.len();
+ let mut value = vec![0; length * 2];
+ let mut i = 0;
+ while i < length {
+ value[i * 2] = (bytes[i] & 0xF0) >> 4;
+ value[i * 2 + 1] = bytes[i] & 0x0F;
+ i += 1;
+ }
+ value
+}
+
+fn hex_string(s: &str) -> Vec<u8> {
+ hex_bytes(s.as_bytes())
+}
+
+fn hex_bytes_to_string(bytes: &[u8]) -> Result<String, std::fmt::Error> {
+ let mut hex_string = String::with_capacity(bytes.len() * 2);
+ for byte in bytes {
+ write!(&mut hex_string, "{:X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed: Vec<Option<String>> = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ let string_array = StringArray::from(hexed);
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Utf8 | DataType::LargeUtf8 => {
+ let array = as_string_array(array);
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_string(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: Vec<Option<String>> = array
+ .iter()
+ .map(|v| v.map(|v| hex_bytes_to_string(&hex_bytes(v))).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let string_array = StringArray::from(hexed);
+
+ Ok(ColumnarValue::Array(Arc::new(string_array)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
Review Comment:
> Do you have any concrete examples of how this works by any chance?
Hmm, it's been a while, I cannot find right away...
BTW my comment above is not a blocker. Since this PR already implemented it, we can follow up separately
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624649245
##########
spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala:
##########
@@ -1038,6 +1038,20 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
}
}
}
+ test("hex") {
+ Seq(true, false).foreach { dictionaryEnabled =>
+ withTempDir { dir =>
+ val path = new Path(dir.toURI.toString, "hex.parquet")
+ makeParquetFileAllTypes(path, dictionaryEnabled = dictionaryEnabled, 10000)
+
+ withParquetTable(path.toString, "tbl") {
+ // _9 and _10 (uint8 and uint16) not supported
+ checkSparkAnswerAndOperator(
+ "SELECT hex(_1), hex(_2), hex(_3), hex(_4), hex(_5), hex(_6), hex(_7), hex(_8), hex(_11), hex(_12), hex(_13), hex(_14), hex(_15), hex(_16), hex(_17), hex(_18), hex(_19), hex(_20) FROM tbl")
Review Comment:
`hex(literal)` should be evaluated to literal early in Spark optimizer. I wonder what test failure you saw?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add hex scalar function [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #449:
URL: https://github.com/apache/datafusion-comet/pull/449#discussion_r1624713927
##########
core/src/execution/datafusion/expressions/scalar_funcs/hex.rs:
##########
@@ -0,0 +1,306 @@
+// 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/LICENSE-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.
+
+use std::sync::Arc;
+
+use arrow::{
+ array::{as_dictionary_array, as_largestring_array, as_string_array},
+ datatypes::Int32Type,
+};
+use arrow_array::StringArray;
+use arrow_schema::DataType;
+use datafusion::logical_expr::ColumnarValue;
+use datafusion_common::{
+ cast::{as_binary_array, as_fixed_size_binary_array, as_int64_array},
+ exec_err, DataFusionError,
+};
+use std::fmt::Write;
+
+fn hex_int64(num: i64) -> String {
+ format!("{:X}", num)
+}
+
+fn hex_bytes<T: AsRef<[u8]>>(bytes: T) -> Result<String, std::fmt::Error> {
+ let bytes = bytes.as_ref();
+ let length = bytes.len();
+ let mut hex_string = String::with_capacity(length * 2);
+ for &byte in bytes {
+ write!(&mut hex_string, "{:02X}", byte)?;
+ }
+ Ok(hex_string)
+}
+
+pub(super) fn spark_hex(args: &[ColumnarValue]) -> Result<ColumnarValue, DataFusionError> {
+ if args.len() != 1 {
+ return Err(DataFusionError::Internal(
+ "hex expects exactly one argument".to_string(),
+ ));
+ }
+
+ match &args[0] {
+ ColumnarValue::Array(array) => match array.data_type() {
+ DataType::Int64 => {
+ let array = as_int64_array(array)?;
+
+ let hexed_array: StringArray = array.iter().map(|v| v.map(hex_int64)).collect();
+
+ Ok(ColumnarValue::Array(Arc::new(hexed_array)))
+ }
+ DataType::Utf8 => {
+ let array = as_string_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::LargeUtf8 => {
+ let array = as_largestring_array(array);
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Binary => {
+ let array = as_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::FixedSizeBinary(_) => {
+ let array = as_fixed_size_binary_array(array)?;
+
+ let hexed: StringArray = array
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ Ok(ColumnarValue::Array(Arc::new(hexed)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Int64) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_int64_array(dict.values())?;
+ let values = hexed_values
+ .iter()
+ .map(|v| v.map(hex_int64))
+ .collect::<Vec<_>>();
+
+ let keys = dict.keys().clone();
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
+ Ok(ColumnarValue::Array(Arc::new(string_array_values)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Utf8) => {
+ let dict = as_dictionary_array::<Int32Type>(&array);
+
+ let hexed_values = as_string_array(dict.values());
+ let values: Vec<Option<String>> = hexed_values
+ .iter()
+ .map(|v| v.map(hex_bytes).transpose())
+ .collect::<Result<_, _>>()?;
+
+ let keys = dict.keys().clone();
+
+ let mut new_keys = Vec::with_capacity(values.len());
+
+ for key in keys.iter() {
+ let key = key.map(|k| values[k as usize].clone()).unwrap_or(None);
+ new_keys.push(key);
+ }
+
+ let string_array_values = StringArray::from(new_keys);
+ Ok(ColumnarValue::Array(Arc::new(string_array_values)))
+ }
+ DataType::Dictionary(_, value_type) if matches!(**value_type, DataType::Binary) => {
Review Comment:
I filed https://github.com/apache/datafusion-comet/issues/504
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org