[GitHub] [arrow] nevi-me commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.

[GitBox <gi...@apache.org>]

nevi-me commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r463935705



##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Defines kernel for length of a string array
+
+use crate::array::*;
+use crate::{
+    datatypes::DataType,
+    error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt64 denoting the number of characters of the array.
+///
+/// * This only accepts StringArray
+/// * Lenght of null is null.
+pub fn length(array: &Array) -> Result<ArrayRef> {
+    match array.data_type() {
+        DataType::Utf8 => {
+            // TODO: is it possible to compute the length directly from the data instead, of value by value?
+            let b = array.as_any().downcast_ref::<StringArray>().unwrap();
+            let mut builder = UInt64Builder::new(b.len());

Review comment:
       Lengths can never be bigger than u32, why not return that?
   
   The lengths of the string are stored in the offsets. So you might be able to avoid branching if you iterate through the offsets to create a non-null Vec<u32> then take the null bitmap from the string array as null values.
   
   I think that might perform better

##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,102 @@
+// 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.
+
+//! Defines kernel for length of a string array
+
+use crate::array::*;
+use crate::{
+    datatypes::DataType,
+    error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt64 denoting the number of characters of the array.
+///
+/// * This only accepts StringArray
+/// * Lenght of null is null.
+pub fn length(array: &Array) -> Result<ArrayRef> {
+    match array.data_type() {
+        DataType::Utf8 => {
+            // TODO: is it possible to compute the length directly from the data instead, of value by value?
+            let b = array.as_any().downcast_ref::<StringArray>().unwrap();
+            let mut builder = UInt64Builder::new(b.len());

Review comment:
       Also, if the offset approach works, it might be useful to extend it to binary and list types.
   
   Instead of this returning an error for primitive types, what about returning 1 for those 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org