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 2020/08/01 07:00:47 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
jorgecarleitao opened a new pull request #7876:
URL: https://github.com/apache/arrow/pull/7876
----------------------------------------------------------------
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 #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#issuecomment-673065004
Thank you @nevi-me , @jhorstmann and @paddyhoran for the reviews, much appreciated!
----------------------------------------------------------------
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 #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r469002405
##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,185 @@
+// 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,
+ datatypes::UInt32Type,
+ error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt32 denoting the number of characters in each string in the array.
+///
+/// * this only accepts StringArray
+/// * lenght of null is null.
+/// * length is in number of bytes
+pub fn length(array: &Array) -> Result<UInt32Array> {
+ match array.data_type() {
+ DataType::Utf8 => {
+ // note: offsets are stored as u8, but they can be interpreted as u32
+ let offsets = array.data_ref().clone().buffers()[0].clone();
+ // this is a 30% improvement over iterating over u8s and building u32
Review comment:
I kept this here as a justification for using `unsafe`. By making it explicit in a comment, an auditor of this code will have an easier time understand why the unsafe is being considered in this particular case, instead of having to spend the time trying to refactor out the `unsafe` only to find a performance drop.
----------------------------------------------------------------
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 #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r463941048
##########
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:
@nevi-me , thanks a lot for the review. I will implement these.
My planned steps
1. u64 -> u32
2. create bench for it
3. play with both cases (offset and non-offset) on the bench.
----------------------------------------------------------------
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] nevi-me closed pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
nevi-me closed pull request #7876:
URL: https://github.com/apache/arrow/pull/7876
----------------------------------------------------------------
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] jhorstmann commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
jhorstmann commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r465857294
##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,185 @@
+// 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,
+ datatypes::UInt32Type,
+ error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt32 denoting the number of characters in each string in the array.
+///
+/// * this only accepts StringArray
+/// * lenght of null is null.
+/// * length of utf8 with more than one code point is the number of code points
Review comment:
Does this really return length in code points or rather in bytes?
----------------------------------------------------------------
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] nevi-me commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by 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? (Maybe it's fine because of large list types)
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
----------------------------------------------------------------
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] nevi-me commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r468581694
##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,185 @@
+// 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,
+ datatypes::UInt32Type,
+ error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt32 denoting the number of characters in each string in the array.
+///
+/// * this only accepts StringArray
+/// * lenght of null is null.
Review comment:
There's a typo here
----------------------------------------------------------------
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 #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#issuecomment-667485474
https://issues.apache.org/jira/browse/ARROW-9615
----------------------------------------------------------------
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] nevi-me commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
nevi-me commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r468582014
##########
File path: rust/arrow/src/compute/kernels/length.rs
##########
@@ -0,0 +1,185 @@
+// 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,
+ datatypes::UInt32Type,
+ error::{ArrowError, Result},
+};
+use std::sync::Arc;
+
+/// Returns an array of UInt32 denoting the number of characters in each string in the array.
+///
+/// * this only accepts StringArray
+/// * lenght of null is null.
+/// * length is in number of bytes
+pub fn length(array: &Array) -> Result<UInt32Array> {
+ match array.data_type() {
+ DataType::Utf8 => {
+ // note: offsets are stored as u8, but they can be interpreted as u32
+ let offsets = array.data_ref().clone().buffers()[0].clone();
+ // this is a 30% improvement over iterating over u8s and building u32
Review comment:
nit: we could probably remove this comment
----------------------------------------------------------------
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 #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #7876:
URL: https://github.com/apache/arrow/pull/7876#discussion_r463994068
##########
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:
I implemented the three points above; the effort (obviously) paid off:
```
length time: [64.937 us 65.779 us 66.891 us]
change: [-59.809% -58.818% -57.761%] (p = 0.00 < 0.05)
Performance has improved.
Found 10 outliers among 100 measurements (10.00%)
3 (3.00%) high mild
7 (7.00%) high severe
```
All the changes are now part of this PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nevi-me commented on a change in pull request #7876: ARROW-9615: [Rust] Added kernel to compute length of a string.
Posted by 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