You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/02/16 21:23:32 UTC

[GitHub] [arrow] seddonm1 opened a new pull request #9509: ARROW-11298: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

seddonm1 opened a new pull request #9509:
URL: https://github.com/apache/arrow/pull/9509


   Splitting up https://github.com/apache/arrow/pull/9243
   
   This implements the following functions:
   
   - String functions
     - [x] bit_Length
     - [x] char_length
     - [x] character_length
     - [x] length
     - [x] octet_length


----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577298569



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -617,48 +670,137 @@ mod tests {
     };
     use arrow::{
         array::{
-            ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
+            Array, ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
             UInt32Array, UInt64Array,
         },
         datatypes::Field,
         record_batch::RecordBatch,
     };
 
-    fn generic_test_math(value: ScalarValue, expected: &str) -> Result<()> {
-        // any type works here: we evaluate against a literal of `value`
-        let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
-        let columns: Vec<ArrayRef> = vec![Arc::new(Int32Array::from(vec![1]))];
-
-        let arg = lit(value);
-
-        let expr = create_physical_expr(&BuiltinScalarFunction::Exp, &[arg], &schema)?;
-
-        // type is correct
-        assert_eq!(expr.data_type(&schema)?, DataType::Float64);
-
-        // evaluate works
-        let batch = RecordBatch::try_new(Arc::new(schema.clone()), columns)?;
-        let result = expr.evaluate(&batch)?.into_array(batch.num_rows());
-
-        // downcast works
-        let result = result.as_any().downcast_ref::<Float64Array>().unwrap();
-
-        // value is correct
-        assert_eq!(result.value(0).to_string(), expected);
-
-        Ok(())
+    /// $FUNC function to test
+    /// $ARGS arguments (vec) to pass to function
+    /// $EXPECTED a Result<Option<$EXPECTED_TYPE>> where Result allows testing errors and Option allows testing Null
+    /// $EXPECTED_TYPE is the expected value type
+    /// $DATA_TYPE is the function to test result type
+    /// $ARRAY_TYPE is the column type after function applied
+    macro_rules! test_function {
+        ($FUNC:ident, $ARGS:expr, $EXPECTED:expr, $EXPECTED_TYPE:ty, $DATA_TYPE: ident, $ARRAY_TYPE:ident) => {
+            println!("{:?}", BuiltinScalarFunction::$FUNC);

Review comment:
       it is just there to help debugging but happy to remove




----------------------------------------------------------------
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

[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577283407



##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -617,48 +670,137 @@ mod tests {
     };
     use arrow::{
         array::{
-            ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
+            Array, ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
             UInt32Array, UInt64Array,
         },
         datatypes::Field,
         record_batch::RecordBatch,
     };
 
-    fn generic_test_math(value: ScalarValue, expected: &str) -> Result<()> {
-        // any type works here: we evaluate against a literal of `value`
-        let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
-        let columns: Vec<ArrayRef> = vec![Arc::new(Int32Array::from(vec![1]))];
-
-        let arg = lit(value);
-
-        let expr = create_physical_expr(&BuiltinScalarFunction::Exp, &[arg], &schema)?;
-
-        // type is correct
-        assert_eq!(expr.data_type(&schema)?, DataType::Float64);
-
-        // evaluate works
-        let batch = RecordBatch::try_new(Arc::new(schema.clone()), columns)?;
-        let result = expr.evaluate(&batch)?.into_array(batch.num_rows());
-
-        // downcast works
-        let result = result.as_any().downcast_ref::<Float64Array>().unwrap();
-
-        // value is correct
-        assert_eq!(result.value(0).to_string(), expected);
-
-        Ok(())
+    /// $FUNC function to test
+    /// $ARGS arguments (vec) to pass to function
+    /// $EXPECTED a Result<Option<$EXPECTED_TYPE>> where Result allows testing errors and Option allows testing Null
+    /// $EXPECTED_TYPE is the expected value type
+    /// $DATA_TYPE is the function to test result type
+    /// $ARRAY_TYPE is the column type after function applied
+    macro_rules! test_function {
+        ($FUNC:ident, $ARGS:expr, $EXPECTED:expr, $EXPECTED_TYPE:ty, $DATA_TYPE: ident, $ARRAY_TYPE:ident) => {
+            println!("{:?}", BuiltinScalarFunction::$FUNC);

Review comment:
       is this `println!` intended?

##########
File path: rust/arrow/src/compute/kernels/bit_length.rs
##########
@@ -0,0 +1,208 @@
+// 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::*, buffer::Buffer};
+use crate::{
+    datatypes::DataType,
+    error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+fn bit_length_string<OffsetSize>(array: &Array, data_type: DataType) -> ArrayRef
+where
+    OffsetSize: OffsetSizeTrait,
+{
+    // note: offsets are stored as u8, but they can be interpreted as OffsetSize
+    let offsets = &array.data_ref().buffers()[0];
+    // this is a 30% improvement over iterating over u8s and building OffsetSize, which
+    // justifies the usage of `unsafe`.
+    let slice: &[OffsetSize] =
+        &unsafe { offsets.typed_data::<OffsetSize>() }[array.offset()..];
+
+    let bit_size = OffsetSize::from_usize(8).unwrap();
+    let lengths = slice
+        .windows(2)
+        .map(|offset| (offset[1] - offset[0]) * bit_size);
+
+    // JUSTIFICATION
+    //  Benefit
+    //      ~60% speedup
+    //  Soundness
+    //      `values` is an iterator with a known size.
+    let buffer = unsafe { Buffer::from_trusted_len_iter(lengths) };
+

Review comment:
       Isn't this a copy of length, with a single difference, that we multiply the lengths by `bit_size`?
   
   If that is the case, can we create a generic or something that avoids code duplication? Copy-pasting code like this is IMO an unnecessary maintenance burden.

##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -115,6 +119,40 @@ where
     }
 }
 
+/// Returns number of characters in the string.
+/// character_length_i32('josé') = 4
+pub fn character_length_i32(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array: &GenericStringArray<i32> = args[0]
+        .as_any()
+        .downcast_ref::<GenericStringArray<i32>>()
+        .unwrap();
+
+    // first map is the iterator, second is for the `Option<_>`
+    let result = string_array
+        .iter()
+        .map(|x| x.map(|x: &str| x.graphemes(true).count() as i32))
+        .collect::<Int32Array>();
+
+    Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Returns number of characters in the string.
+/// character_length_i64('josé') = 4
+pub fn character_length_i64(args: &[ArrayRef]) -> Result<ArrayRef> {

Review comment:
       I think that this code can be reduced to a single generic. Something like this (untested):
   
   ```rust
   pub fn character_length<T: ArrowNativeType>(args: &[ArrayRef]) -> Result<ArrayRef> 
   where 
   T::Native: StringOffsetSizeTrait {
       let string_array: &GenericStringArray<T::Native> = args[0]
           .as_any()
           .downcast_ref::<GenericStringArray<T::Native>>()
           .unwrap();
       
        string_array
           .iter()
           .map(|x| x.map(|x: &str| T::Native::from_usize(x.graphemes(true).count())).unwrap())
           .collect()
   }
   ```
   




----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780271885


   Thanks @jorgecarleitao . I will update soon.


----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780378219


   @jorgecarleitao Thanks for the review. I have addressed all of your points.
   
   The `T::Native: StringOffsetSizeTrait` will be useful in some of the other string functions so 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.

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

[GitHub] [arrow] codecov-io edited a comment on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780849543


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=h1) Report
   > Merging [#9509](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=desc) (17740e9) into [master](https://codecov.io/gh/apache/arrow/commit/e2d6c057684b587151afffe50f7eaef94533e017?el=desc) (e2d6c05) will **increase** coverage by `0.12%`.
   > The diff coverage is `88.05%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9509/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9509      +/-   ##
   ==========================================
   + Coverage   82.15%   82.28%   +0.12%     
   ==========================================
     Files         243      244       +1     
     Lines       54908    55517     +609     
   ==========================================
   + Hits        45108    45680     +572     
   - Misses       9800     9837      +37     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `72.36% <68.49%> (-3.95%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `81.13% <80.00%> (+0.94%)` | :arrow_up: |
   | [rust/arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2xlbmd0aC5ycw==) | `100.00% <100.00%> (ø)` | |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `69.62% <100.00%> (+2.49%)` | :arrow_up: |
   | [rust/arrow/src/array/equal/utils.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvZXF1YWwvdXRpbHMucnM=) | `75.49% <0.00%> (-0.99%)` | :arrow_down: |
   | [rust/arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvcmVjb3JkX2JhdGNoLnJz) | `83.70% <0.00%> (-0.63%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/limit.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2xpbWl0LnJz) | `57.47% <0.00%> (-0.49%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `79.58% <0.00%> (-0.05%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `82.45% <0.00%> (-0.04%)` | :arrow_down: |
   | [rust/benchmarks/src/bin/tpch.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9iZW5jaG1hcmtzL3NyYy9iaW4vdHBjaC5ycw==) | `38.33% <0.00%> (ø)` | |
   | ... and [12 more](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=footer). Last update [e2d6c05...17740e9](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [arrow] jorgecarleitao edited a comment on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
jorgecarleitao edited a comment on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780701060


   Thanks a lot, @seddonm1 . I went through this PR again and I pull the code locally to understand the reason for the macro.
   
   I now get it that it is not easy to write this. :) I PRed something to your branch with a suggestion to reduce the risk of using that function (which is unsafe due to the `typed`) on non-string arrays and make it just use generics, that imo are usually easier to read because the constraints are specific.


----------------------------------------------------------------
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

[GitHub] [arrow] alamb commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-782663789


   @seddonm1  we are so close -- now there is a clippy error: https://github.com/apache/arrow/pull/9509/checks?check_run_id=1922005826
   
   ```
   ror: this function's return value is unnecessarily wrapped by `Result`
      --> arrow/src/compute/kernels/length.rs:86:1
       |
   86  | / fn bit_length_impl<O: StringOffsetSizeTrait, T: ArrowPrimitiveType>(
   87  | |     array: &dyn Array,
   88  | | ) -> Result<ArrayRef>
   89  | | where
   ...   |
   99  | |     }))
   100 | | }
       | |_^
       |
       = note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
   help: remove `Result` from the return type...
       |
   88  | ) -> std::sync::Arc<(dyn array::array::Array + 'static)>
       |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   help: ...and change the returning expressions
       |
   97  |     unary_offsets_string::<O, _>(array, T::DATA_TYPE, |x| {
   98  |         x * bits_in_bytes
   99  |     })
       |
   
   error: aborting due to previous error
   
   error: could not compile `arrow`
   
   To learn more, run the command again with --verbose.
   warning: build failed, waiting for other jobs to finish...
   error: this function's return value is unnecessarily wrapped by `Result`
      --> arrow/src/compute/kernels/length.rs:86:1
       |
   86  | / fn bit_length_impl<O: StringOffsetSizeTrait, T: ArrowPrimitiveType>(
   87  | |     array: &dyn Array,
   88  | | ) -> Result<ArrayRef>
   89  | | where
   ...   |
   99  | |     }))
   100 | | }
       | |_^
       |
       = note: `-D clippy::unnecessary-wraps` implied by `-D warnings`
       = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_wraps
   help: remove `Result` from the return type...
       |
   88  | ) -> std::sync::Arc<(dyn array::array::Array + 'static)>
       |      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   help: ...and change the returning expressions
       |
   97  |     unary_offsets_string::<O, _>(array, T::DATA_TYPE, |x| {
   98  |         x * bits_in_bytes
   99  |     })
       |
   ```


----------------------------------------------------------------
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

[GitHub] [arrow] codecov-io commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780849543


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=h1) Report
   > Merging [#9509](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=desc) (619f2f6) into [master](https://codecov.io/gh/apache/arrow/commit/e2d6c057684b587151afffe50f7eaef94533e017?el=desc) (e2d6c05) will **increase** coverage by `0.09%`.
   > The diff coverage is `88.32%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9509/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9509      +/-   ##
   ==========================================
   + Coverage   82.15%   82.24%   +0.09%     
   ==========================================
     Files         243      244       +1     
     Lines       54908    55414     +506     
   ==========================================
   + Hits        45108    45577     +469     
   - Misses       9800     9837      +37     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `81.13% <ø> (+0.94%)` | :arrow_up: |
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `72.36% <68.49%> (-3.95%)` | :arrow_down: |
   | [rust/arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2xlbmd0aC5ycw==) | `100.00% <100.00%> (ø)` | |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `69.62% <100.00%> (+2.49%)` | :arrow_up: |
   | [rust/datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `82.45% <0.00%> (-0.04%)` | :arrow_down: |
   | [rust/benchmarks/src/bin/tpch.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9iZW5jaG1hcmtzL3NyYy9iaW4vdHBjaC5ycw==) | `38.33% <0.00%> (ø)` | |
   | [rust/datafusion/src/optimizer/constant\_folding.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9vcHRpbWl6ZXIvY29uc3RhbnRfZm9sZGluZy5ycw==) | `94.46% <0.00%> (ø)` | |
   | [rust/datafusion/src/sql/planner.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9zcWwvcGxhbm5lci5ycw==) | `83.23% <0.00%> (+0.07%)` | :arrow_up: |
   | [rust/datafusion/src/optimizer/utils.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9vcHRpbWl6ZXIvdXRpbHMucnM=) | `52.39% <0.00%> (+0.36%)` | :arrow_up: |
   | [rust/datafusion/src/execution/context.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9leGVjdXRpb24vY29udGV4dC5ycw==) | `90.84% <0.00%> (+0.40%)` | :arrow_up: |
   | ... and [2 more](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=footer). Last update [e2d6c05...619f2f6](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [arrow] jorgecarleitao commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780701060


   Thanks a lot, @seddonm1 . I went through this PR again and I pull the code locally to understand the reason for the macro.
   
   I now get it that it is not easy to write this. :) I PRed something to your branch with a suggestion to reduce the risk of using that function (which is unsafe due to the `typed`) on non-string arrays.


----------------------------------------------------------------
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

[GitHub] [arrow] codecov-io edited a comment on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780849543


   # [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=h1) Report
   > Merging [#9509](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=desc) (76855f2) into [master](https://codecov.io/gh/apache/arrow/commit/e2d6c057684b587151afffe50f7eaef94533e017?el=desc) (e2d6c05) will **increase** coverage by `0.12%`.
   > The diff coverage is `88.11%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/arrow/pull/9509/graphs/tree.svg?width=650&height=150&src=pr&token=LpTCFbqVT1)](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9509      +/-   ##
   ==========================================
   + Coverage   82.15%   82.28%   +0.12%     
   ==========================================
     Files         243      244       +1     
     Lines       54908    55518     +610     
   ==========================================
   + Hits        45108    45681     +573     
   - Misses       9800     9837      +37     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [rust/datafusion/src/physical\_plan/functions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2Z1bmN0aW9ucy5ycw==) | `72.36% <68.49%> (-3.95%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/expr.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vZXhwci5ycw==) | `81.13% <80.00%> (+0.94%)` | :arrow_up: |
   | [rust/arrow/src/compute/kernels/length.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvY29tcHV0ZS9rZXJuZWxzL2xlbmd0aC5ycw==) | `100.00% <100.00%> (ø)` | |
   | [...datafusion/src/physical\_plan/string\_expressions.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3N0cmluZ19leHByZXNzaW9ucy5ycw==) | `69.62% <100.00%> (+2.49%)` | :arrow_up: |
   | [rust/arrow/src/array/equal/utils.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvYXJyYXkvZXF1YWwvdXRpbHMucnM=) | `75.49% <0.00%> (-0.99%)` | :arrow_down: |
   | [rust/arrow/src/record\_batch.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9hcnJvdy9zcmMvcmVjb3JkX2JhdGNoLnJz) | `83.70% <0.00%> (-0.63%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/limit.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL2xpbWl0LnJz) | `57.47% <0.00%> (-0.49%)` | :arrow_down: |
   | [rust/datafusion/src/physical\_plan/planner.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9waHlzaWNhbF9wbGFuL3BsYW5uZXIucnM=) | `79.58% <0.00%> (-0.05%)` | :arrow_down: |
   | [rust/datafusion/src/logical\_plan/plan.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9kYXRhZnVzaW9uL3NyYy9sb2dpY2FsX3BsYW4vcGxhbi5ycw==) | `82.45% <0.00%> (-0.04%)` | :arrow_down: |
   | [rust/benchmarks/src/bin/tpch.rs](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree#diff-cnVzdC9iZW5jaG1hcmtzL3NyYy9iaW4vdHBjaC5ycw==) | `38.33% <0.00%> (ø)` | |
   | ... and [12 more](https://codecov.io/gh/apache/arrow/pull/9509/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=footer). Last update [e2d6c05...76855f2](https://codecov.io/gh/apache/arrow/pull/9509?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-781611124


   @jorgecarleitao Sorry I updated this but forgot to update you. Once this is merged I can do more of the big Postgres string functions PRs.


----------------------------------------------------------------
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

[GitHub] [arrow] github-actions[bot] commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780167407


   https://issues.apache.org/jira/browse/ARROW-11651


----------------------------------------------------------------
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

[GitHub] [arrow] alamb commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-782835332


   Thanks again @seddonm1  -- this is epic work


----------------------------------------------------------------
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

[GitHub] [arrow] github-actions[bot] commented on pull request #9509: ARROW-11298: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-780126538


   https://issues.apache.org/jira/browse/ARROW-11298


----------------------------------------------------------------
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

[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577285930



##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -115,6 +119,40 @@ where
     }
 }
 
+/// Returns number of characters in the string.
+/// character_length_i32('josé') = 4
+pub fn character_length_i32(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array: &GenericStringArray<i32> = args[0]
+        .as_any()
+        .downcast_ref::<GenericStringArray<i32>>()
+        .unwrap();
+
+    // first map is the iterator, second is for the `Option<_>`
+    let result = string_array
+        .iter()
+        .map(|x| x.map(|x: &str| x.graphemes(true).count() as i32))
+        .collect::<Int32Array>();
+
+    Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Returns number of characters in the string.
+/// character_length_i64('josé') = 4
+pub fn character_length_i64(args: &[ArrayRef]) -> Result<ArrayRef> {

Review comment:
       I think that this code can be reduced to a single generic. Something like this (untested):
   
   ```rust
   pub fn character_length<T: ArrowPrimitiveType>(args: &[ArrayRef]) -> Result<ArrayRef> 
   where 
   T::Native: StringOffsetSizeTrait {
       let string_array: &GenericStringArray<T::Native> = args[0]
           .as_any()
           .downcast_ref::<GenericStringArray<T::Native>>()
           .unwrap();
       
        string_array
           .iter()
           .map(|x| x.map(|x: &str| T::Native::from_usize(x.graphemes(true).count())).unwrap())
           .collect()
   }
   ```
   




----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577299127



##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -115,6 +119,40 @@ where
     }
 }
 
+/// Returns number of characters in the string.
+/// character_length_i32('josé') = 4
+pub fn character_length_i32(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array: &GenericStringArray<i32> = args[0]
+        .as_any()
+        .downcast_ref::<GenericStringArray<i32>>()
+        .unwrap();
+
+    // first map is the iterator, second is for the `Option<_>`
+    let result = string_array
+        .iter()
+        .map(|x| x.map(|x: &str| x.graphemes(true).count() as i32))
+        .collect::<Int32Array>();
+
+    Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Returns number of characters in the string.
+/// character_length_i64('josé') = 4
+pub fn character_length_i64(args: &[ArrayRef]) -> Result<ArrayRef> {

Review comment:
       Thanks I will have a go. There are a couple more instances this generic from_usize could help.




----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-782760774


   @alamb done and ready. Thanks to @jorgecarleitao for his elite knowledge of trait implementations.


----------------------------------------------------------------
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

[GitHub] [arrow] alamb closed pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
alamb closed pull request #9509:
URL: https://github.com/apache/arrow/pull/9509


   


----------------------------------------------------------------
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

[GitHub] [arrow] alamb commented on pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#issuecomment-782086690


   The integration test failure appears unrelated https://github.com/apache/arrow/pull/9509/checks?check_run_id=1921983282
   
   I will restart it to try and get a clean run
   
   ```
   
   [INFO] Final Memory: 176M/780M
   [INFO] ------------------------------------------------------------------------
   Error:  Failed to execute goal org.xolstice.maven.plugins:protobuf-maven-plugin:0.5.0:compile (src) on project flight-core: Execution src of goal org.xolstice.maven.plugins:protobuf-maven-plugin:0.5.0:compile failed: Plugin org.xolstice.maven.plugins:protobuf-maven-plugin:0.5.0 or one of its dependencies could not be resolved: Could not transfer artifact org.apache.maven.plugin-tools:maven-plugin-annotations:jar:3.4 from/to central (https://repo.maven.apache.org/maven2): Connection reset -> [Help 1]
   Error:  
   Error:  To see the full stack trace of the errors, re-run Maven with the -e switch.
   Error:  Re-run Maven using the -X switch to enable full debug logging.
   Error:  
   Error:  For more information about the errors and possible solutions, please read the following articles:
   Error:  [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/PluginResolutionException
   Error:  
   Error:  After correcting the problems, you can resume the build with the command
   Error:    mvn <goals> -rf :flight-core
   Destroying 3 processes
   Destroying process..
   Destroying process..
   Destroying process..
   ```


----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577395791



##########
File path: rust/datafusion/src/physical_plan/string_expressions.rs
##########
@@ -115,6 +119,40 @@ where
     }
 }
 
+/// Returns number of characters in the string.
+/// character_length_i32('josé') = 4
+pub fn character_length_i32(args: &[ArrayRef]) -> Result<ArrayRef> {
+    let string_array: &GenericStringArray<i32> = args[0]
+        .as_any()
+        .downcast_ref::<GenericStringArray<i32>>()
+        .unwrap();
+
+    // first map is the iterator, second is for the `Option<_>`
+    let result = string_array
+        .iter()
+        .map(|x| x.map(|x: &str| x.graphemes(true).count() as i32))
+        .collect::<Int32Array>();
+
+    Ok(Arc::new(result) as ArrayRef)
+}
+
+/// Returns number of characters in the string.
+/// character_length_i64('josé') = 4
+pub fn character_length_i64(args: &[ArrayRef]) -> Result<ArrayRef> {

Review comment:
       Wow. Impressive you can come up with this in your head. I have incorporated this 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.

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

[GitHub] [arrow] seddonm1 commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577395453



##########
File path: rust/arrow/src/compute/kernels/bit_length.rs
##########
@@ -0,0 +1,208 @@
+// 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::*, buffer::Buffer};
+use crate::{
+    datatypes::DataType,
+    error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+fn bit_length_string<OffsetSize>(array: &Array, data_type: DataType) -> ArrayRef
+where
+    OffsetSize: OffsetSizeTrait,
+{
+    // note: offsets are stored as u8, but they can be interpreted as OffsetSize
+    let offsets = &array.data_ref().buffers()[0];
+    // this is a 30% improvement over iterating over u8s and building OffsetSize, which
+    // justifies the usage of `unsafe`.
+    let slice: &[OffsetSize] =
+        &unsafe { offsets.typed_data::<OffsetSize>() }[array.offset()..];
+
+    let bit_size = OffsetSize::from_usize(8).unwrap();
+    let lengths = slice
+        .windows(2)
+        .map(|offset| (offset[1] - offset[0]) * bit_size);
+
+    // JUSTIFICATION
+    //  Benefit
+    //      ~60% speedup
+    //  Soundness
+    //      `values` is an iterator with a known size.
+    let buffer = unsafe { Buffer::from_trusted_len_iter(lengths) };
+

Review comment:
       I have created a macro allowing just the core function to be replaced.

##########
File path: rust/datafusion/src/physical_plan/functions.rs
##########
@@ -617,48 +670,137 @@ mod tests {
     };
     use arrow::{
         array::{
-            ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
+            Array, ArrayRef, FixedSizeListArray, Float64Array, Int32Array, StringArray,
             UInt32Array, UInt64Array,
         },
         datatypes::Field,
         record_batch::RecordBatch,
     };
 
-    fn generic_test_math(value: ScalarValue, expected: &str) -> Result<()> {
-        // any type works here: we evaluate against a literal of `value`
-        let schema = Schema::new(vec![Field::new("a", DataType::Int32, false)]);
-        let columns: Vec<ArrayRef> = vec![Arc::new(Int32Array::from(vec![1]))];
-
-        let arg = lit(value);
-
-        let expr = create_physical_expr(&BuiltinScalarFunction::Exp, &[arg], &schema)?;
-
-        // type is correct
-        assert_eq!(expr.data_type(&schema)?, DataType::Float64);
-
-        // evaluate works
-        let batch = RecordBatch::try_new(Arc::new(schema.clone()), columns)?;
-        let result = expr.evaluate(&batch)?.into_array(batch.num_rows());
-
-        // downcast works
-        let result = result.as_any().downcast_ref::<Float64Array>().unwrap();
-
-        // value is correct
-        assert_eq!(result.value(0).to_string(), expected);
-
-        Ok(())
+    /// $FUNC function to test
+    /// $ARGS arguments (vec) to pass to function
+    /// $EXPECTED a Result<Option<$EXPECTED_TYPE>> where Result allows testing errors and Option allows testing Null
+    /// $EXPECTED_TYPE is the expected value type
+    /// $DATA_TYPE is the function to test result type
+    /// $ARRAY_TYPE is the column type after function applied
+    macro_rules! test_function {
+        ($FUNC:ident, $ARGS:expr, $EXPECTED:expr, $EXPECTED_TYPE:ty, $DATA_TYPE: ident, $ARRAY_TYPE:ident) => {
+            println!("{:?}", BuiltinScalarFunction::$FUNC);

Review comment:
       Removed.




----------------------------------------------------------------
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

[GitHub] [arrow] seddonm1 commented on a change in pull request #9509: ARROW-11651: [Rust][DataFusion] Implement Postgres String Functions: Length Functions

Posted by GitBox <gi...@apache.org>.
seddonm1 commented on a change in pull request #9509:
URL: https://github.com/apache/arrow/pull/9509#discussion_r577298518



##########
File path: rust/arrow/src/compute/kernels/bit_length.rs
##########
@@ -0,0 +1,208 @@
+// 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::*, buffer::Buffer};
+use crate::{
+    datatypes::DataType,
+    error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+fn bit_length_string<OffsetSize>(array: &Array, data_type: DataType) -> ArrayRef
+where
+    OffsetSize: OffsetSizeTrait,
+{
+    // note: offsets are stored as u8, but they can be interpreted as OffsetSize
+    let offsets = &array.data_ref().buffers()[0];
+    // this is a 30% improvement over iterating over u8s and building OffsetSize, which
+    // justifies the usage of `unsafe`.
+    let slice: &[OffsetSize] =
+        &unsafe { offsets.typed_data::<OffsetSize>() }[array.offset()..];
+
+    let bit_size = OffsetSize::from_usize(8).unwrap();
+    let lengths = slice
+        .windows(2)
+        .map(|offset| (offset[1] - offset[0]) * bit_size);
+
+    // JUSTIFICATION
+    //  Benefit
+    //      ~60% speedup
+    //  Soundness
+    //      `values` is an iterator with a known size.
+    let buffer = unsafe { Buffer::from_trusted_len_iter(lengths) };
+

Review comment:
       yes i can have a look at doing that.




----------------------------------------------------------------
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