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/09/01 06:59:35 UTC
[GitHub] [arrow] jorgecarleitao opened a new pull request #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
jorgecarleitao opened a new pull request #8090:
URL: https://github.com/apache/arrow/pull/8090
----------------------------------------------------------------
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 a change in pull request #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r481068711
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// 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.
+
+//! String expressions
+
+use crate::error::{ExecutionError, Result};
+use arrow::array::{Array, ArrayRef, StringArray, StringBuilder};
+
+macro_rules! downcast_vec {
+ ($ARGS:expr, $ARRAY_TYPE:ident) => {{
+ $ARGS
+ .iter()
+ .map(|e| match e.as_any().downcast_ref::<$ARRAY_TYPE>() {
+ Some(array) => Ok(array),
+ _ => Err(ExecutionError::General("failed to downcast".to_string())),
+ })
+ }};
+}
+
+/// concatenate string columns together.
+pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
+ // downcast all arguments to strings
+ let args = downcast_vec!(args, StringArray).collect::<Result<Vec<&StringArray>>>()?;
+ // do not accept 0 arguments.
+ assert!(args.len() != 0);
Review comment:
I would prefer that we not commit code that can `panic`s -- so in this PR I would suggest doing one of the following
1. Add an error in physical planing for `concat()`
2. Implement `concat()`
Here is one possible behavior for `concat()`:
1. The result value is always the Null
2. It should produce one output value for each input value
A potential implementation could be "rewrite `concat()` to be `concat("")` -- aka have the planner insert a single constant argument
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r480896532
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review comment:
I created a new one to not over-populate `expressions.rs`. If we accept this, we should prob. move `length` 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] andygrove commented on pull request #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
andygrove commented on pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#issuecomment-685913096
@jorgecarleitao This one needs rebasing 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r481070644
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -682,6 +682,32 @@ fn query_length() -> Result<()> {
Ok(())
}
+#[test]
+fn query_concat() -> Result<()> {
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("c1", DataType::Utf8, false),
+ Field::new("c2", DataType::Int32, true),
+ ]));
+
+ let data = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(vec!["", "a", "aa", "aaa"])),
+ Arc::new(Int32Array::from(vec![Some(0), Some(1), None, Some(3)])),
+ ],
+ )?;
+
+ let table = MemTable::new(schema, vec![vec![data]])?;
+
+ let mut ctx = ExecutionContext::new();
+ ctx.register_table("test", Box::new(table));
+ let sql = "SELECT concat(c1, '-hi-', cast(c2 as varchar)) FROM test";
+ let actual = execute(&mut ctx, sql);
+ let expected = vec!["\"-hi-0\"", "\"a-hi-1\"", "\"\"", "\"aaa-hi-3\""];
Review comment:
I am surprised that the third result is `""` and not `NULL` (as the third i32 input is NULL). Is that some artifact of using `execute`?
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r481275359
##########
File path: rust/datafusion/tests/sql.rs
##########
@@ -682,6 +682,32 @@ fn query_length() -> Result<()> {
Ok(())
}
+#[test]
+fn query_concat() -> Result<()> {
+ let schema = Arc::new(Schema::new(vec![
+ Field::new("c1", DataType::Utf8, false),
+ Field::new("c2", DataType::Int32, true),
+ ]));
+
+ let data = RecordBatch::try_new(
+ schema.clone(),
+ vec![
+ Arc::new(StringArray::from(vec!["", "a", "aa", "aaa"])),
+ Arc::new(Int32Array::from(vec![Some(0), Some(1), None, Some(3)])),
+ ],
+ )?;
+
+ let table = MemTable::new(schema, vec![vec![data]])?;
+
+ let mut ctx = ExecutionContext::new();
+ ctx.register_table("test", Box::new(table));
+ let sql = "SELECT concat(c1, '-hi-', cast(c2 as varchar)) FROM test";
+ let actual = execute(&mut ctx, sql);
+ let expected = vec!["\"-hi-0\"", "\"a-hi-1\"", "\"\"", "\"aaa-hi-3\""];
Review comment:
Yeah, it took me some time to figure that out also; it is an artifact from `execute`. I changed its behavior to output NULL, but I think we may need to reformulate that code at some point, as only `utf8` show `NULL`.
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r480902988
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// 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.
+
+//! String expressions
+
+use crate::error::{ExecutionError, Result};
+use arrow::array::{Array, ArrayRef, StringArray, StringBuilder};
+
+macro_rules! downcast_vec {
+ ($ARGS:expr, $ARRAY_TYPE:ident) => {{
+ $ARGS
+ .iter()
+ .map(|e| match e.as_any().downcast_ref::<$ARRAY_TYPE>() {
+ Some(array) => Ok(array),
+ _ => Err(ExecutionError::General("failed to downcast".to_string())),
+ })
+ }};
+}
+
+/// concatenate string columns together.
+pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
+ // downcast all arguments to strings
+ let args = downcast_vec!(args, StringArray).collect::<Result<Vec<&StringArray>>>()?;
+ // do not accept 0 arguments.
+ assert!(args.len() != 0);
Review comment:
this currently panics on zero arguments, which means that the plan `SELECT c1, concat() FROM t` crashes on run-time.
We should prob. safe-guard this during planning
Essentially, all our physical expressions currently require one argument or they do not know how many rows to return.
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// 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.
+
+//! String expressions
+
+use crate::error::{ExecutionError, Result};
+use arrow::array::{Array, ArrayRef, StringArray, StringBuilder};
+
+macro_rules! downcast_vec {
+ ($ARGS:expr, $ARRAY_TYPE:ident) => {{
+ $ARGS
+ .iter()
+ .map(|e| match e.as_any().downcast_ref::<$ARRAY_TYPE>() {
+ Some(array) => Ok(array),
+ _ => Err(ExecutionError::General("failed to downcast".to_string())),
+ })
+ }};
+}
+
+/// concatenate string columns together.
+pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
+ // downcast all arguments to strings
+ let args = downcast_vec!(args, StringArray).collect::<Result<Vec<&StringArray>>>()?;
+ // do not accept 0 arguments.
+ assert!(args.len() != 0);
Review comment:
this currently panics on zero arguments, which means that the plan `SELECT c1, concat() FROM t` crashes on run-time.
We should prob. safe-guard this during planning
Essentially, all our physical expressions currently require at least argument or they do not know how many rows to return.
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r480902988
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// 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.
+
+//! String expressions
+
+use crate::error::{ExecutionError, Result};
+use arrow::array::{Array, ArrayRef, StringArray, StringBuilder};
+
+macro_rules! downcast_vec {
+ ($ARGS:expr, $ARRAY_TYPE:ident) => {{
+ $ARGS
+ .iter()
+ .map(|e| match e.as_any().downcast_ref::<$ARRAY_TYPE>() {
+ Some(array) => Ok(array),
+ _ => Err(ExecutionError::General("failed to downcast".to_string())),
+ })
+ }};
+}
+
+/// concatenate string columns together.
+pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
+ // downcast all arguments to strings
+ let args = downcast_vec!(args, StringArray).collect::<Result<Vec<&StringArray>>>()?;
+ // do not accept 0 arguments.
+ assert!(args.len() != 0);
Review comment:
this currently panics on zero arguments, which means that the plan `SELECT c1, concat() FROM t` crashes on run-time.
We should prob. safe-guard this in type coercion.
Essentially, all our physical expressions currently require one argument or they do not know how many rows to return.
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
jorgecarleitao commented on a change in pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#discussion_r481274249
##########
File path: rust/datafusion/src/execution/physical_plan/string_expressions.rs
##########
@@ -0,0 +1,63 @@
+// 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.
+
+//! String expressions
+
+use crate::error::{ExecutionError, Result};
+use arrow::array::{Array, ArrayRef, StringArray, StringBuilder};
+
+macro_rules! downcast_vec {
+ ($ARGS:expr, $ARRAY_TYPE:ident) => {{
+ $ARGS
+ .iter()
+ .map(|e| match e.as_any().downcast_ref::<$ARRAY_TYPE>() {
+ Some(array) => Ok(array),
+ _ => Err(ExecutionError::General("failed to downcast".to_string())),
+ })
+ }};
+}
+
+/// concatenate string columns together.
+pub fn concatenate(args: &[ArrayRef]) -> Result<StringArray> {
+ // downcast all arguments to strings
+ let args = downcast_vec!(args, StringArray).collect::<Result<Vec<&StringArray>>>()?;
+ // do not accept 0 arguments.
+ assert!(args.len() != 0);
Review comment:
Thanks for the suggestions 👍 I've added an error when the plan has 0 arguments.
----------------------------------------------------------------
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 #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #8090:
URL: https://github.com/apache/arrow/pull/8090#issuecomment-684501657
https://issues.apache.org/jira/browse/ARROW-9892
----------------------------------------------------------------
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] andygrove closed pull request #8090: ARROW-9892: [Rust] [DataFusion] Added concat of utf8
Posted by GitBox <gi...@apache.org>.
andygrove closed pull request #8090:
URL: https://github.com/apache/arrow/pull/8090
----------------------------------------------------------------
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