alamb commented on code in PR #9743:
URL: https://github.com/apache/arrow-datafusion/pull/9743#discussion_r1536794183
##########
datafusion/sqllogictest/test_files/struct.slt:
##########
@@ -50,6 +50,12 @@ select struct(1, 3.14, 'e');
----
{c0: 1, c1: 3.14, c2: e}
+# struct scalar function #1 with alias
+query ?
+select struct(1 as "name0", 3.14 as name1, 'e', true as 'name3');
Review Comment:
this is very cool -- I didn't realize we supported this syntax
##########
datafusion/functions/src/core/named_struct.rs:
##########
@@ -0,0 +1,186 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::StructArray;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::{ColumnarValue, Expr, ExprSchemable};
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+
+/// put values in a struct array.
+fn named_struct_expr(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ // do not accept 0 arguments.
+ if args.is_empty() {
+ return exec_err!("named_struct requires at least one pair of
arguments");
+ }
+
+ if args.len() % 2 != 0 {
+ return exec_err!("named_struct requires an even number of arguments");
+ }
+
+ let fields = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let name = &chunk[0];
+ let value = &chunk[1];
+
+ if let ColumnarValue::Scalar(ScalarValue::Utf8(Some(name))) = name
{
+ let array_ref = match value {
+ ColumnarValue::Array(array) => array.clone(),
+ ColumnarValue::Scalar(scalar) =>
scalar.to_array()?.clone(),
+ };
+
+ Ok((
+ Arc::new(Field::new(
+ name.clone(),
+ array_ref.data_type().clone(),
+ true,
+ )),
+ array_ref,
+ ))
+ } else {
+ exec_err!("named_struct even arguments must be string
literals")
+ }
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ColumnarValue::Array(Arc::new(StructArray::from(fields))))
+}
+#[derive(Debug)]
+pub(super) struct NamedStructFunc {
+ signature: Signature,
+}
+
+impl NamedStructFunc {
+ pub fn new() -> Self {
+ Self {
+ signature: Signature::variadic_any(Volatility::Immutable),
+ }
+ }
+}
+
+impl ScalarUDFImpl for NamedStructFunc {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+ fn name(&self) -> &str {
+ "named_struct"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ Err(datafusion_common::DataFusionError::Internal(
+ "return_type called instead of return_type_from_exprs".into(),
+ ))
+ }
+
+ fn return_type_from_exprs(
+ &self,
+ args: &[datafusion_expr::Expr],
+ schema: &dyn datafusion_common::ExprSchema,
+ _arg_types: &[DataType],
+ ) -> Result<DataType> {
+ // do not accept 0 arguments.
+ if args.is_empty() {
+ return exec_err!("named_struct requires at least one pair of
arguments");
+ }
+
+ if args.len() % 2 != 0 {
+ return exec_err!("named_struct requires an even number of
arguments");
+ }
+
+ let return_fields = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let name = &chunk[0];
+ let value = &chunk[1];
+
+ if let Expr::Literal(ScalarValue::Utf8(Some(name))) = name {
+ Ok(Field::new(name.clone(), value.get_type(schema)?, true))
+ } else {
+ exec_err!("named_struct even arguments must be string
literals")
Review Comment:
I think it is valuable here to provide details on what the user got instead:
```suggestion
exec_err!("named_struct even arguments must be string
literals, got {}", name)
```
##########
datafusion/functions/src/core/named_struct.rs:
##########
@@ -0,0 +1,186 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::StructArray;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::{ColumnarValue, Expr, ExprSchemable};
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+
+/// put values in a struct array.
+fn named_struct_expr(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ // do not accept 0 arguments.
+ if args.is_empty() {
+ return exec_err!("named_struct requires at least one pair of
arguments");
+ }
+
+ if args.len() % 2 != 0 {
+ return exec_err!("named_struct requires an even number of arguments");
+ }
+
+ let fields = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let name = &chunk[0];
+ let value = &chunk[1];
+
+ if let ColumnarValue::Scalar(ScalarValue::Utf8(Some(name))) = name
{
+ let array_ref = match value {
+ ColumnarValue::Array(array) => array.clone(),
+ ColumnarValue::Scalar(scalar) =>
scalar.to_array()?.clone(),
+ };
+
+ Ok((
+ Arc::new(Field::new(
+ name.clone(),
+ array_ref.data_type().clone(),
+ true,
+ )),
+ array_ref,
+ ))
+ } else {
+ exec_err!("named_struct even arguments must be string
literals")
Review Comment:
Similarly to below, I think it would be valuable to explictly describe here
what was received instead so a user who encounters this error will more easily
be able to find their mistake and correct it
##########
datafusion/functions/src/core/named_struct.rs:
##########
@@ -0,0 +1,186 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::StructArray;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::{ColumnarValue, Expr, ExprSchemable};
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+
+/// put values in a struct array.
Review Comment:
Rather than adding a new function, I think we could instead change the
existing `struct` UDF to support explicit named fields.
Perhaps we can do that as a follow on PR?
##########
datafusion/functions/src/core/named_struct.rs:
##########
@@ -0,0 +1,186 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::StructArray;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::{ColumnarValue, Expr, ExprSchemable};
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+
+/// put values in a struct array.
+fn named_struct_expr(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ // do not accept 0 arguments.
+ if args.is_empty() {
+ return exec_err!("named_struct requires at least one pair of
arguments");
+ }
+
+ if args.len() % 2 != 0 {
+ return exec_err!("named_struct requires an even number of arguments");
+ }
+
+ let fields = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let name = &chunk[0];
+ let value = &chunk[1];
+
+ if let ColumnarValue::Scalar(ScalarValue::Utf8(Some(name))) = name
{
+ let array_ref = match value {
+ ColumnarValue::Array(array) => array.clone(),
+ ColumnarValue::Scalar(scalar) =>
scalar.to_array()?.clone(),
+ };
+
+ Ok((
+ Arc::new(Field::new(
+ name.clone(),
+ array_ref.data_type().clone(),
+ true,
+ )),
+ array_ref,
+ ))
+ } else {
+ exec_err!("named_struct even arguments must be string
literals")
+ }
+ })
+ .collect::<Result<Vec<_>>>()?;
+ Ok(ColumnarValue::Array(Arc::new(StructArray::from(fields))))
+}
+#[derive(Debug)]
+pub(super) struct NamedStructFunc {
+ signature: Signature,
+}
+
+impl NamedStructFunc {
+ pub fn new() -> Self {
+ Self {
+ signature: Signature::variadic_any(Volatility::Immutable),
+ }
+ }
+}
+
+impl ScalarUDFImpl for NamedStructFunc {
+ fn as_any(&self) -> &dyn Any {
+ self
+ }
+ fn name(&self) -> &str {
+ "named_struct"
+ }
+
+ fn signature(&self) -> &Signature {
+ &self.signature
+ }
+
+ fn return_type(&self, _arg_types: &[DataType]) -> Result<DataType> {
+ Err(datafusion_common::DataFusionError::Internal(
+ "return_type called instead of return_type_from_exprs".into(),
+ ))
Review Comment:
Can we use `internal_err!` here and also include the name `named_struct` as
part of the error message?
##########
datafusion/sqllogictest/test_files/struct.slt:
##########
@@ -63,10 +69,10 @@ query TT
explain select struct(a, b, c) from values;
----
logical_plan
-Projection: struct(values.a, values.b, values.c)
+Projection: named_struct(Utf8("c0"), values.a, Utf8("c1"), values.b,
Utf8("c2"), values.c)
--TableScan: values projection=[a, b, c]
physical_plan
-ProjectionExec: expr=[struct(a@0, b@1, c@2) as
struct(values.a,values.b,values.c)]
+ProjectionExec: expr=[named_struct(c0, a@0, c1, b@1, c2, c@2) as
named_struct(Utf8("c0"),values.a,Utf8("c1"),values.b,Utf8("c2"),values.c)]
--MemoryExec: partitions=1, partition_sizes=[1]
statement ok
Review Comment:
Can we please add a few more tests:
1. Error case: named_struct with 0 arguments
2. Error case: named_struct wth an odd number of arguments
3. Error case: named_struct with an even number of arguments
4. A test where the arguments are both columns and arrays (example below)
Here is an example (I expect it to panic in this PR at first -- see
https://github.com/apache/arrow-datafusion/pull/9775 for how to fix it)
```
❯ create table t(x int) as values (1), (2), (3);
0 rows in set. Query took 0.014 seconds.
❯ select named_struct(x as 'col_x', 25 as scalar) from t;
```
##########
datafusion/functions/src/core/named_struct.rs:
##########
@@ -0,0 +1,186 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+use arrow::array::StructArray;
+use arrow::datatypes::{DataType, Field, Fields};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::{ColumnarValue, Expr, ExprSchemable};
+use datafusion_expr::{ScalarUDFImpl, Signature, Volatility};
+use std::any::Any;
+use std::sync::Arc;
+
+/// put values in a struct array.
+fn named_struct_expr(args: &[ColumnarValue]) -> Result<ColumnarValue> {
+ // do not accept 0 arguments.
+ if args.is_empty() {
+ return exec_err!("named_struct requires at least one pair of
arguments");
+ }
+
+ if args.len() % 2 != 0 {
+ return exec_err!("named_struct requires an even number of arguments");
+ }
+
+ let fields = args
+ .chunks_exact(2)
+ .map(|chunk| {
+ let name = &chunk[0];
+ let value = &chunk[1];
+
+ if let ColumnarValue::Scalar(ScalarValue::Utf8(Some(name))) = name
{
+ let array_ref = match value {
+ ColumnarValue::Array(array) => array.clone(),
+ ColumnarValue::Scalar(scalar) =>
scalar.to_array()?.clone(),
Review Comment:
I think you need to turn this into the correct length to match any array
arguments (I mention a test below that will fix the problem) otherwise the
sizes of the arguments will be mismatched
I suggest using `ColumnarValues::values_to_array` if possible
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
