You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2023/08/19 01:05:34 UTC
[GitHub] [arrow-datafusion] jayzhan211 opened a new pull request, #7338: Support nulls and empty in array functions
jayzhan211 opened a new pull request, #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #7142.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1301993038
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -543,6 +543,77 @@ fn coerce_arguments_for_signature(
.collect::<Result<Vec<_>>>()
}
+// TODO: Add this function to arrow-rs
+fn get_list_base_type(data_type: &DataType) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(_) => get_list_base_type(field.data_type()),
+ base_type => Ok(base_type.clone()),
+ },
+
+ _ => Ok(data_type.clone()),
+ }
+}
+
+fn get_list_from_base_type(
+ data_type: &DataType,
+ base_type: &DataType,
+) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(inner_field) => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ get_list_from_base_type(inner_field.data_type(), base_type)?,
+ field.is_nullable(),
+ )))),
+ _ => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ base_type.clone(),
+ field.is_nullable(),
+ )))),
+ },
+
+ _ => Ok(base_type.clone()),
+ }
+}
+
+fn coerce_nulls_for_array_append(
Review Comment:
Perhaps you could create a function like
```rust
/// Given elements of input_types, returns the single field DataType
/// such that a `DataType::List` with that field type can store each element after coercion
//
// For example, given an [Int32, Int64, and NULL] this function would return Int64
// meaning that the elements could all be stored in a `DataType::List(Int64)`
// (after first being cast to Int64)`
fn coerce_array_args(input_types: Vec<DataType>) -> Result<DataType> {
...
}
```
I think this would generalize simply to lists of lists but I haven't thought about it myself
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -543,6 +543,77 @@ fn coerce_arguments_for_signature(
.collect::<Result<Vec<_>>>()
}
+// TODO: Add this function to arrow-rs
+fn get_list_base_type(data_type: &DataType) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(_) => get_list_base_type(field.data_type()),
+ base_type => Ok(base_type.clone()),
+ },
+
+ _ => Ok(data_type.clone()),
+ }
+}
+
+fn get_list_from_base_type(
+ data_type: &DataType,
+ base_type: &DataType,
+) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(inner_field) => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ get_list_from_base_type(inner_field.data_type(), base_type)?,
+ field.is_nullable(),
+ )))),
+ _ => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ base_type.clone(),
+ field.is_nullable(),
+ )))),
+ },
+
+ _ => Ok(base_type.clone()),
+ }
+}
+
+fn coerce_nulls_for_array_append(
Review Comment:
Perhaps you could create a function like
```rust
/// Given elements of input_types, returns the single field DataType
/// such that a `DataType::List` with that field type can store each element after coercion
//
// For example, given an [Int32, Int64, and NULL] this function would return Int64
// meaning that the elements could all be stored in a `DataType::List(Int64)`
// (after first being cast to Int64)`
fn coerce_array_args(input_types: Vec<DataType>) -> Result<DataType> {
...
}
```
I think this would generalize simply to lists of lists but I haven't thought about it deeply
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1303659919
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
values.iter().map(|e| e.get_datatype()).collect();
- if data_types.is_empty() {
- Ok(lit(ScalarValue::new_list(None, DataType::Utf8)))
- } else if data_types.len() > 1 {
- Err(DataFusionError::NotImplemented(format!(
- "Arrays with different types are not supported: {data_types:?}",
- )))
- } else {
- let data_type = values[0].get_datatype();
+ match data_types.len() {
+ 0 => Ok(lit(ScalarValue::new_list(None, DataType::Utf8))),
+ 1 => {
+ let data_type = values[0].get_datatype();
+
+ Ok(lit(ScalarValue::new_list(Some(values), data_type)))
+ }
+ // Check whether one of them is null
+ 2 => {
Review Comment:
> I didn't sure what is the case for >2. If you have int, float, and null in arrays, int, and float should be coerced before null handling, so I think only null and one data type are expected.
False statement
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1684889091
@izveigor @alamb Ready for review, thanls~
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on pull request #7338: Support nulls and empty for array functions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1697226593
> Hello, @jayzhan211!
> Sorry for the long delay. I will carefully review these changes tomorrow.
>
> However, I have some questions about the nature of functions in Arrow Datafusion. I recently thought about solving the issue: https://github.com/apache/arrow-datafusion/issues/6559 and I noticed a fact that User Defined Functions in DataFusion does not parameterize the null handling behaviour (Unlike DuckDB with `special` [link](https://duckdb.org/docs/api/python/function.html#null-handling)) So If we want to implement new array or other nested data types functions (like `struct_insert`) maybe it is better to remake signature's structure (I mean adding new boolean argument `special` to `Signature`).
>
> I want to hear @alamb's opinion about this opportunity.
I'm not familiar with udf. Is it possible to customize the behavior of null in udf? Why do we need additional parameters to deal with 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on pull request #7338: Support nulls and empty for array functions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1696149093
Hello, @jayzhan211!
Sorry for the long delay. I will carefully review these changes tomorrow.
However, I have some questions about the nature of functions in Arrow Datafusion. I recently thought about solving the issue: https://github.com/apache/arrow-datafusion/issues/6559 and I noticed a fact that User Defined Functions in DataFusion does not parameterize the null handling behaviour (Unlike DuckDB with `special` [link](https://duckdb.org/docs/api/python/function.html#null-handling)) So If we want to implement new array or other nested data types functions (like `struct_insert`) maybe it is better to remake signature's structure (I mean adding new boolean argument `special` to `Signature`).
I want to hear @alamb's opinion about this opportunity.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7338: Support nulls and empty for array functions
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1697308652
I plan to review this PR later -- I am on vacation this week so my response will likely be delayed
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #7338: Support nulls and empty for array append and prepend
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1686847731
I'll put this on my list -- thanks @jayzhan211
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1301979259
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
values.iter().map(|e| e.get_datatype()).collect();
- if data_types.is_empty() {
- Ok(lit(ScalarValue::new_list(None, DataType::Utf8)))
- } else if data_types.len() > 1 {
- Err(DataFusionError::NotImplemented(format!(
- "Arrays with different types are not supported: {data_types:?}",
- )))
- } else {
- let data_type = values[0].get_datatype();
+ match data_types.len() {
+ 0 => Ok(lit(ScalarValue::new_list(None, DataType::Utf8))),
+ 1 => {
+ let data_type = values[0].get_datatype();
+
+ Ok(lit(ScalarValue::new_list(Some(values), data_type)))
+ }
+ // Check whether one of them is null
+ 2 => {
+ let has_null_type = data_types.contains(&DataType::Null);
+ if has_null_type {
+ // convert to Vec for indexing
+ let data_types = data_types.into_iter().collect::<Vec<_>>();
+ let non_null_type = if data_types[0] == DataType::Null {
Review Comment:
How do we know the Null is the first or second type? Maybe you could use something like
```
let non_null_type = data_types.find(|d| d == DataType::Null)
```
or something
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
Review Comment:
Yeah, I agree the use of a `HashSet` here is going to be tough because it will lose the argument order (and thus you can't pick the first non null type deterministically
What about keeping track of the types in both the map and a Vec, like
```
let data_types: Vec<DataType>= vec![];
let seen_types: HashSet<DataType> = HashSet::new();
```
Then I think you can use `data_types` below as you'll know it is non duplicated
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -543,6 +543,77 @@ fn coerce_arguments_for_signature(
.collect::<Result<Vec<_>>>()
}
+// TODO: Add this function to arrow-rs
+fn get_list_base_type(data_type: &DataType) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(_) => get_list_base_type(field.data_type()),
+ base_type => Ok(base_type.clone()),
+ },
+
+ _ => Ok(data_type.clone()),
+ }
+}
+
+fn get_list_from_base_type(
+ data_type: &DataType,
+ base_type: &DataType,
+) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(inner_field) => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ get_list_from_base_type(inner_field.data_type(), base_type)?,
+ field.is_nullable(),
+ )))),
+ _ => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ base_type.clone(),
+ field.is_nullable(),
+ )))),
+ },
+
+ _ => Ok(base_type.clone()),
+ }
+}
+
+fn coerce_nulls_for_array_append(
Review Comment:
I dont understand what this function is trying to do and I don't understand why the rules for array_append would be different than the rules for array scalars.
could you add some comments or maybe try to unify the implementations somehow?
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
values.iter().map(|e| e.get_datatype()).collect();
- if data_types.is_empty() {
- Ok(lit(ScalarValue::new_list(None, DataType::Utf8)))
- } else if data_types.len() > 1 {
- Err(DataFusionError::NotImplemented(format!(
- "Arrays with different types are not supported: {data_types:?}",
- )))
- } else {
- let data_type = values[0].get_datatype();
+ match data_types.len() {
+ 0 => Ok(lit(ScalarValue::new_list(None, DataType::Utf8))),
+ 1 => {
+ let data_type = values[0].get_datatype();
+
+ Ok(lit(ScalarValue::new_list(Some(values), data_type)))
+ }
+ // Check whether one of them is null
+ 2 => {
Review Comment:
Why does this stop at 2? Why not handle 3 or more different data types?
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -907,6 +927,16 @@ select list_push_back(make_array(1, 2, 3), 4), list_push_back(make_array(1.0, 2.
----
[1, 2, 3, 4] [1.0, 2.0, 3.0, 4.0] [h, e, l, l, o]
+query ?
+select array_append([1, 2, 3], null);
Review Comment:
Can we also please add a test with more than two types, for example:
```sql
select [1, '2', Null]
```
Which postgres does
```sql
postgres=# select ARRAY [ 1, '2', NULL ];
array
------------
{1,2,NULL}
(1 row)
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7338: Support nulls and empty for array functions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1308283800
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -553,6 +553,198 @@ fn coerce_arguments_for_signature(
.collect::<Result<Vec<_>>>()
}
+// TODO: Move this function to arrow-rs or common array utils module
+// base type is the non-list type
+fn base_type(data_type: &DataType) -> Result<DataType> {
Review Comment:
The function definitely has a practical use but it should be expanded for all nested data types (list, fixed_list, map, union ...).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1303120889
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
values.iter().map(|e| e.get_datatype()).collect();
- if data_types.is_empty() {
- Ok(lit(ScalarValue::new_list(None, DataType::Utf8)))
- } else if data_types.len() > 1 {
- Err(DataFusionError::NotImplemented(format!(
- "Arrays with different types are not supported: {data_types:?}",
- )))
- } else {
- let data_type = values[0].get_datatype();
+ match data_types.len() {
+ 0 => Ok(lit(ScalarValue::new_list(None, DataType::Utf8))),
+ 1 => {
+ let data_type = values[0].get_datatype();
+
+ Ok(lit(ScalarValue::new_list(Some(values), data_type)))
+ }
+ // Check whether one of them is null
+ 2 => {
+ let has_null_type = data_types.contains(&DataType::Null);
+ if has_null_type {
+ // convert to Vec for indexing
+ let data_types = data_types.into_iter().collect::<Vec<_>>();
+ let non_null_type = if data_types[0] == DataType::Null {
Review Comment:
I assume there is only null and `one` non-null, if not, this fails
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on pull request #7338: Support nulls and empty for array functions
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#issuecomment-1694596965
I have extensively restructured the code in order to enhance its readability and extensibility. Also, `array_concat` is considered.
I'm sure this PR can be further improved but it is time to seek feedback to prevent me from working in the wrong direction.
cc @alamb @izveigor
> Not sure why Clippy fails, I pass it locally.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1303136228
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -543,6 +543,77 @@ fn coerce_arguments_for_signature(
.collect::<Result<Vec<_>>>()
}
+// TODO: Add this function to arrow-rs
+fn get_list_base_type(data_type: &DataType) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(_) => get_list_base_type(field.data_type()),
+ base_type => Ok(base_type.clone()),
+ },
+
+ _ => Ok(data_type.clone()),
+ }
+}
+
+fn get_list_from_base_type(
+ data_type: &DataType,
+ base_type: &DataType,
+) -> Result<DataType> {
+ match data_type {
+ DataType::List(field) => match field.data_type() {
+ DataType::List(inner_field) => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ get_list_from_base_type(inner_field.data_type(), base_type)?,
+ field.is_nullable(),
+ )))),
+ _ => Ok(DataType::List(Arc::new(Field::new(
+ field.name(),
+ base_type.clone(),
+ field.is_nullable(),
+ )))),
+ },
+
+ _ => Ok(base_type.clone()),
+ }
+}
+
+fn coerce_nulls_for_array_append(
Review Comment:
Actually, I just try to deal with append and prepend. I have not thought deeply about other array functions yet, so not sure if they have similar coercion, If yes, having one function for them is pretty nice.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1303683293
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -907,6 +927,16 @@ select list_push_back(make_array(1, 2, 3), 4), list_push_back(make_array(1.0, 2.
----
[1, 2, 3, 4] [1.0, 2.0, 3.0, 4.0] [h, e, l, l, o]
+query ?
+select array_append([1, 2, 3], null);
Review Comment:
It seems I would need casting between scalar value
ScalarValue I64(1) -> ScalarValue F64(1.0)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Support nulls and empty for array functions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7338:
URL: https://github.com/apache/datafusion/pull/7338#issuecomment-2092010757
Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Support nulls and empty for array functions [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7338: Support nulls and empty for array functions
URL: https://github.com/apache/datafusion/pull/7338
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
[GitHub] [arrow-datafusion] izveigor commented on a diff in pull request #7338: Support nulls and empty for array functions
Posted by "izveigor (via GitHub)" <gi...@apache.org>.
izveigor commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1308288920
##########
datafusion/optimizer/src/analyzer/type_coercion.rs:
##########
@@ -581,48 +773,22 @@ fn coerce_arguments_for_fun(
.collect::<Result<Vec<_>>>()?;
}
- if *fun == BuiltinScalarFunction::MakeArray {
- // Find the final data type for the function arguments
- let current_types = expressions
- .iter()
- .map(|e| e.get_type(schema))
- .collect::<Result<Vec<_>>>()?;
-
- let new_type = current_types
- .iter()
- .skip(1)
- .fold(current_types.first().unwrap().clone(), |acc, x| {
- comparison_coercion(&acc, x).unwrap_or(acc)
- });
-
- return expressions
- .iter()
- .zip(current_types)
- .map(|(expr, from_type)| cast_array_expr(expr, &from_type, &new_type, schema))
- .collect();
+ match fun {
Review Comment:
I am inclined to solve this problem by expanding signature's structure because there is one difficulty with User Defined Function. For example, I am Arrow DataFusion's user and I want to define my own `ArrayAppend` implementation (the function `new_array_append`). And how this function would handle nulls?
What do you think about it, @alamb and @jayzhan211?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] jayzhan211 commented on a diff in pull request #7338: Support nulls and empty for array append and prepend
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #7338:
URL: https://github.com/apache/arrow-datafusion/pull/7338#discussion_r1303119419
##########
datafusion/sql/src/expr/value.rs:
##########
@@ -151,16 +151,51 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
let data_types: HashSet<DataType> =
values.iter().map(|e| e.get_datatype()).collect();
- if data_types.is_empty() {
- Ok(lit(ScalarValue::new_list(None, DataType::Utf8)))
- } else if data_types.len() > 1 {
- Err(DataFusionError::NotImplemented(format!(
- "Arrays with different types are not supported: {data_types:?}",
- )))
- } else {
- let data_type = values[0].get_datatype();
+ match data_types.len() {
+ 0 => Ok(lit(ScalarValue::new_list(None, DataType::Utf8))),
+ 1 => {
+ let data_type = values[0].get_datatype();
+
+ Ok(lit(ScalarValue::new_list(Some(values), data_type)))
+ }
+ // Check whether one of them is null
+ 2 => {
Review Comment:
I didn't sure what is the case for >2.
If you have int, float, and null in arrays, int, and float should be coerced before null handling, so I think only null and one data type are expected.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org