You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Weijun-H (via GitHub)" <gi...@apache.org> on 2024/02/02 08:10:21 UTC
[PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Weijun-H opened a new pull request, #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108
## 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.
-->
Follow #8902
## 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
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#issuecomment-1964111313
@Weijun-H, thanks again!
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` , `array_positions` should use it.
```
D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
D SELECT list_position(null, 1);
┌────────────────────────┐
│ list_position(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501935059
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1063,7 +1116,7 @@ from arrays_values_without_nulls;
## array_element (aliases: array_extract, list_extract, list_element)
# array_element error
-query error DataFusion error: Error during planning: No function matches the given name and argument types 'array_element\(Int64, Int64\)'. You might need to add explicit type casts.\n\tCandidate functions:\n\tarray_element\(array, index\)
Review Comment:
The previous error shows the expected argument, it seems better than signature name itself
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501348925
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
Maybe we can just ignore nulls as first argument since we are not coerce them to any other type. Unlike, array_append(arr, null) as second argument, in this case we will convert to other primitives.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501934556
##########
datafusion/expr/src/signature.rs:
##########
@@ -133,25 +136,168 @@ pub enum ArrayFunctionSignature {
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
ArrayAndIndex,
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_element_and_optional_index(
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ // make sure there's 2 or 3 arguments
+ if !(current_types.len() == 2 || current_types.len() == 3) {
+ return Ok(vec![vec![]]);
+ }
+
+ let first_two_types = ¤t_types[0..2];
+ let mut valid_types =
+ array_append_or_prepend_valid_types(first_two_types, true)?;
+
+ // Early return if there are only 2 arguments
+ if current_types.len() == 2 {
+ return Ok(valid_types);
+ }
+
+ let valid_types_with_index = valid_types
+ .iter()
+ .map(|t| {
+ let mut t = t.clone();
+ t.push(DataType::Int64);
+ t
+ })
+ .collect::<Vec<_>>();
+
+ valid_types.extend(valid_types_with_index);
+
+ Ok(valid_types)
+ }
+
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let new_array_type =
+ datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match new_array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let new_elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![new_array_type.clone(), new_elem_type.clone()]])
+ } else {
+ Ok(vec![vec![new_elem_type.to_owned(), new_array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+ let array_type = ¤t_types[0];
+ let array_type = array(&[array_type.clone()])?;
+ if array_type[0].is_empty() {
+ Ok(vec![vec![]])
+ } else {
+ Ok(vec![vec![array_type[0][0].clone(), DataType::Int64]])
+ }
+ }
+ fn array(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 1 {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ match array_type {
+ DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _) => {
+ let array_type = coerced_fixed_size_list_to_list(array_type);
+ Ok(vec![vec![array_type]])
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ match self {
+ ArrayFunctionSignature::ArrayAndElement => {
+ array_append_or_prepend_valid_types(current_types, true)
+ }
+ ArrayFunctionSignature::ElementAndArray => {
+ array_append_or_prepend_valid_types(current_types, false)
+ }
+ ArrayFunctionSignature::ArrayAndIndex => array_and_index(current_types),
+ ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
+ array_element_and_optional_index(current_types)
+ }
+ ArrayFunctionSignature::Array => array(current_types),
+ }
+ }
}
impl std::fmt::Display for ArrayFunctionSignature {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ArrayFunctionSignature::ArrayAndElement => {
- write!(f, "array, element")
Review Comment:
why is this change?
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499278949
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,35 +126,211 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
+ ElementAndArray(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
- ArrayAndIndex,
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_element_and_optional_index(
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ // make sure there's 2 or 3 arguments
+ if !(current_types.len() == 2 || current_types.len() == 3) {
+ return Ok(vec![vec![]]);
+ }
+
+ let first_two_types = ¤t_types[0..2];
+ let mut valid_types =
+ array_append_or_prepend_valid_types(first_two_types, true, true)?;
+
+ // Early return if there are only 2 arguments
+ if current_types.len() == 2 {
+ return Ok(valid_types);
+ }
+
+ let valid_types_with_index = valid_types
+ .iter()
+ .map(|t| {
+ let mut t = t.clone();
+ t.push(DataType::Int64);
+ t
+ })
+ .collect::<Vec<_>>();
+
+ valid_types.extend(valid_types_with_index);
+
+ Ok(valid_types)
+ }
+
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) {
+ if allow_null_coercion {
+ return Ok(vec![vec![array_type.clone(), elem_type.clone()]]);
+ } else {
+ return Ok(vec![vec![]]);
+ }
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let new_array_type =
+ datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match new_array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let new_elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![new_array_type.clone(), new_elem_type.clone()]])
+ } else {
+ Ok(vec![vec![new_elem_type.to_owned(), new_array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
+ return Ok(vec![vec![]]);
+ }
+
+ match array_type {
+ DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _) => {
+ let array_type = coerced_fixed_size_list_to_list(array_type);
+ Ok(vec![vec![array_type, DataType::Int64]])
Review Comment:
I still think it can be further simplied if there is a function that process `coerced_fixed_size_list_to_list` and return `array_type`. Then can be used both in `array_and_index` and `array`
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499396259
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
> in coercion step could filter if the function accepts the NULL type while in array_expressions it determines what the function should return
Does it mean we still need to do the null handling in `array_expression`? Take `array_append` for example, if we find null, we convert it to i64(None) and process it later on.
ideally, nulls should be handled in type coercion. And, `array_expression` can be *simple* that just focuses on processing the result and not handling type or signature-related issues.
If doing the checking again in `array_expression` is just because it is a public function, I think we should come out with a way that the checking can be somewhat reused in both places. Before that, maybe we just leave the checking in `coercion` step.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493721760
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
is there any array function's signature is `array_and_element(true, )`? If not, it is better not to introduce `allow_null_coercion` for 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` , `array_positions`, `array_remove` and `array_remove_all`should use it.
```
D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
D SELECT list_position(null, 1);
┌────────────────────────┐
│ list_position(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
**Schema (PostgreSQL v15)**
---
**Query #1**
SELECT array_remove(NULL, 2);
| array_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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501352016
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
I think that we should only focus on *type coercion* in TypeCoercionRewriter. If we got null, but not casting it to other, ignore it.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499260206
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
why do we need to handle null here (any other array functions)? Is it like handling length checking? If it is, note that `array_element(null, )` should be an error.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499260206
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
why do we need to handle null here? Is it like handling length checking? If it is, note that `array_element(null, )` should be an error.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501732814
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
> I believe our primary focus within TypeCoercionRewriter should be strictly on type coercion. If encountering a null value that isn't being cast to another type, it should be disregarded.
I cannot agree more with that. I should focus on the type coercion, and leave `NULL` aside now. So I will remove `allow_null_coercion ` and don't check `null` 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#issuecomment-1945639094
It seems to support FixedSizeList, we need all the array functions to go through `coerced_fixed_size_list_to_list` once 🤔. I'm not sure it is a good idea, but I did not come out with a better one though
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1484833816
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
Review Comment:
since we are passing allow_null_coercion here, maybe it would make sense to have allow_null_coercion to be part of each array enum? Perhaps something like
```rust
impl ArrayFunctionSignature {
ArrayAndElement(allow_null_coercion),
...
Array(allow_null_coercion)
}
```
But again I am not sure about the need for allow_null_coercion 🤔
##########
datafusion/expr/src/signature.rs:
##########
@@ -117,7 +122,8 @@ pub enum TypeSignature {
/// is `OneOf(vec![Any(0), VariadicAny])`.
OneOf(Vec<TypeSignature>),
/// Specifies Signatures for array functions
- ArraySignature(ArrayFunctionSignature),
+ /// Boolean value specifies whether null type coercion is allowed
+ ArraySignature(ArrayFunctionSignature, bool),
Review Comment:
Could you expand this comment to explain what null type coercion is and when it would make sense *not*n to be allowed?
I don't understand what it is used for 🤔 it seems to me like null coercion should always be allowed
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
Review Comment:
Can we explain what this is in more detail?
Does it mean something like
```suggestion
/// Specialized Signature for ArrayEmpty and similar functions
/// The function takes a single argument that must be a List/LargeList/FixedSizeList
/// or something that can be coerced to one of those types
```
?
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
Review Comment:
is there any reason to create these inner functions, rather than actual functions (or maybe just inline the body of the functions into the `match` statement below)?
I find the extra level of indenting somewhat confusing
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -80,73 +78,6 @@ fn get_valid_types(
signature: &TypeSignature,
current_types: &[DataType],
) -> Result<Vec<Vec<DataType>>> {
- fn array_append_or_prepend_valid_types(
Review Comment:
I see -- so one part of this PR is extracting this function into ArrayFunctionSignature ?
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1485709509
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
Review Comment:
It is only for the `match` statement below
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1485710375
##########
datafusion/expr/src/signature.rs:
##########
@@ -132,7 +138,136 @@ pub enum ArrayFunctionSignature {
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
+ /// Specialized Signature for ArrayEqual and similar functions
ArrayAndIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ allow_null_coercion: bool,
Review Comment:
`allow_null_coercion` determines if the signature can allow a `null` type. For example, `empty(NULL)` accepts `NULL` while `array_dims(Null)` will return error
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501932831
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -929,18 +930,18 @@ impl BuiltinScalarFunction {
// 0 or more arguments of arbitrary type
Signature::one_of(vec![VariadicEqual, Any(0)], self.volatility())
}
- BuiltinScalarFunction::ArrayPopFront => Signature::any(1, self.volatility()),
- BuiltinScalarFunction::ArrayPopBack => Signature::any(1, self.volatility()),
+ BuiltinScalarFunction::ArrayPopFront => Signature::array(self.volatility()),
+ BuiltinScalarFunction::ArrayPopBack => Signature::array(self.volatility()),
BuiltinScalarFunction::ArrayConcat => {
Signature::variadic_any(self.volatility())
}
- BuiltinScalarFunction::ArrayDims => Signature::any(1, self.volatility()),
- BuiltinScalarFunction::ArrayEmpty => Signature::any(1, self.volatility()),
+ BuiltinScalarFunction::ArrayDims => Signature::array(self.volatility()),
Review Comment:
test is not covered
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490720419
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4296,10 +4384,10 @@ select
array_ndims(column4)
from array_ndims_table;
----
-0 1 2 5
-0 1 2 5
-0 1 2 5
-0 1 2 5
+1 1 2 5
+1 1 2 5
+NULL 1 2 5
Review Comment:
It should be an error here too, or just remove the first column
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` should use it.
```
D SELECT list_contains(null, [1]);
┌─────────────────────────────────────────┐
│ list_contains(NULL, main.list_value(1)) │
│ int32 │
├─────────────────────────────────────────┤
│ │
└─────────────────────────────────────────┘
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499285788
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,35 +126,211 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
+ ElementAndArray(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
- ArrayAndIndex,
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
Review Comment:
Actually I don't think we need `get_type_signature`.
Instead, having separate functions for coercion. And, wrap it in the matching function.
This way the function can be reused easily. For example,
```rust
fn array() return array_type.
match {
signature::array => { vec![vec![array()]] }
signature::array_and_index => { vec![vec![array(), i64]] }
}
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1494205691
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4296,10 +4384,10 @@ select
array_ndims(column4)
from array_ndims_table;
----
-0 1 2 5
-0 1 2 5
-0 1 2 5
-0 1 2 5
+1 1 2 5
+1 1 2 5
+NULL 1 2 5
Review Comment:
I testes these cases in PostgreSQL, so I think it should not be a error here
```
**Schema (PostgreSQL v15)**
CREATE TABLE array_ndims_table (
arr_data int[]
);
INSERT INTO array_ndims_table (arr_data)
VALUES
(array[1]),
(array[2]),
(null);
---
**Query #1**
SELECT array_ndims(arr_data) AS array_dimensions
FROM array_ndims_table;
| array_dimensions |
| ---------------- |
| 1 |
| 1 |
| |
---
[View on DB Fiddle](https://www.db-fiddle.com/)
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501320634
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
> > in coercion step could filter if the function accepts the NULL type while in array_expressions it determines what the function should return
>
> Does it mean we still need to do the null handling in `array_expression`? Take `array_append` for example, if we find null, we convert it to i64(None) and process it later on.
>
> ideally, nulls should be handled in type coercion. And, `array_expression` can be _simple_ that just focuses on processing the result and not handling type or signature-related issues.
>
It would be ideal if we could handle null in one place. However, array functions have different results for `NULL`. For instance, `array_append(null, 1)` returns an error, whereas `array_has(null, 1)` returns 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
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1497347650
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -931,51 +932,63 @@ impl BuiltinScalarFunction {
Signature::variadic_any(self.volatility())
}
BuiltinScalarFunction::ArrayAppend => {
- Signature::array_and_element(self.volatility())
+ Signature::array_and_element(false, self.volatility())
}
BuiltinScalarFunction::MakeArray => {
// 0 or more arguments of arbitrary type
Signature::one_of(vec![VariadicEqual, Any(0)], self.volatility())
}
- BuiltinScalarFunction::ArrayPopFront => Signature::any(1, self.volatility()),
- BuiltinScalarFunction::ArrayPopBack => Signature::any(1, self.volatility()),
+ BuiltinScalarFunction::ArrayPopFront => {
+ Signature::array(true, self.volatility())
Review Comment:
Done
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499250893
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -86,7 +86,9 @@ fn compare_element_to_list(
row_index: usize,
eq: bool,
) -> Result<BooleanArray> {
- if list_array_row.data_type() != element_array.data_type() {
+ if list_array_row.data_type() != element_array.data_type()
+ && !element_array.data_type().is_null()
Review Comment:
why should we need to check null 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499273870
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
I think handling null here makes it confusing where it is nulls handled, is it handled in`coercion` step `coerce signature` or here (array_expressions). The test in array.slt only process once. We can only know it is handled correctly, but unsure which one does the job.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499260206
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
why do we need to handle null here (any other array functions)? Is it like pre-checking? If it is, note that `array_element(null, )` should be an error.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490652019
##########
datafusion/expr/src/built_in_function.rs:
##########
@@ -931,51 +932,63 @@ impl BuiltinScalarFunction {
Signature::variadic_any(self.volatility())
}
BuiltinScalarFunction::ArrayAppend => {
- Signature::array_and_element(self.volatility())
+ Signature::array_and_element(false, self.volatility())
}
BuiltinScalarFunction::MakeArray => {
// 0 or more arguments of arbitrary type
Signature::one_of(vec![VariadicEqual, Any(0)], self.volatility())
}
- BuiltinScalarFunction::ArrayPopFront => Signature::any(1, self.volatility()),
- BuiltinScalarFunction::ArrayPopBack => Signature::any(1, self.volatility()),
+ BuiltinScalarFunction::ArrayPopFront => {
+ Signature::array(true, self.volatility())
Review Comment:
I didn't find the fixedsizelist test in array.slt, it seems `Signature::array` is used for it
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` , `array_positions`, `array_remove` and `array_remove_all`should use it.
```
D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
D SELECT list_position(null, 1);
┌────────────────────────┐
│ list_position(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
```
```
**Schema (PostgreSQL v15)**
---
**Query #1**
SELECT array_remove(NULL, 2);
| array_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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499285788
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,35 +126,211 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
+ ElementAndArray(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
- ArrayAndIndex,
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
Review Comment:
Actually, I don't think we need to place all signature functions into `get_type_signature`.
Instead, having separate signature functions. And, collect what we need in the matching function.
This way the function can be reused easily. For example,
```rust
fn array() return array_type.
match {
signature::array => { vec![vec![array()]] }
signature::array_and_index => { vec![vec![array(), i64]] }
}
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499330683
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
> I think handling null here makes it confusing where it is nulls handled, is it handled in`coercion` step `coerce signature` or here (array_expressions). The test in array.slt only process once. We can only know it is handled correctly, but unsure which one does the job.
This is what I am considering now, in `coercion` step could filter if the function accepts the `NULL` type while in `array_expressions` it determines what the function should return @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
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493721760
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
is there any array function's signature is `array_and_element(true, )`? If not, it is better not to introduce `allow_null_coercion` for `array_and_element` 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1494602269
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4296,10 +4384,10 @@ select
array_ndims(column4)
from array_ndims_table;
----
-0 1 2 5
-0 1 2 5
-0 1 2 5
-0 1 2 5
+1 1 2 5
+1 1 2 5
+NULL 1 2 5
Review Comment:
You are right, I thought it should be an error just like array_ndims(NULL) is
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490720419
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4296,10 +4384,10 @@ select
array_ndims(column4)
from array_ndims_table;
----
-0 1 2 5
-0 1 2 5
-0 1 2 5
-0 1 2 5
+1 1 2 5
+1 1 2 5
+NULL 1 2 5
Review Comment:
It should be an error here too
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1502226849
##########
datafusion/expr/src/signature.rs:
##########
@@ -272,11 +254,28 @@ impl ArrayFunctionSignature {
ArrayFunctionSignature::ElementAndArray => {
array_append_or_prepend_valid_types(current_types, false)
}
- ArrayFunctionSignature::ArrayAndIndex => array_and_index(current_types),
+ ArrayFunctionSignature::ArrayAndIndex => {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+ array(¤t_types[0]).map_or_else(
+ || Ok(vec![vec![]]),
+ |array_type| Ok(vec![vec![array_type, DataType::Int64]]),
Review Comment:
👍
##########
datafusion/expr/src/type_coercion/functions.rs:
##########
@@ -208,21 +110,9 @@ fn get_valid_types(
}
TypeSignature::Exact(valid_types) => vec![valid_types.clone()],
- TypeSignature::ArraySignature(ref function_signature) => match function_signature
- {
- ArrayFunctionSignature::ArrayAndElement => {
- return array_append_or_prepend_valid_types(current_types, true)
- }
- ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
- return array_element_and_optional_index(current_types)
- }
- ArrayFunctionSignature::ArrayAndIndex => {
- return array_and_index(current_types)
- }
- ArrayFunctionSignature::ElementAndArray => {
- return array_append_or_prepend_valid_types(current_types, false)
- }
- },
+ TypeSignature::ArraySignature(ref function_signature) => {
+ function_signature.get_type_signature(current_types)?
Review Comment:
We may not need `get_type_signature` but inline matching function instead
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1475706777
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4252,21 +4252,22 @@ NULL [3] [4]
# array_ndims scalar function #1
-query III
+query error
+selrct array_ndims(1), array_ndims(null)
Review Comment:
follow PostgreSQL
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499263824
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
We may need functions like `pre-checking`, or `validation` for the Array function when we move to UDF-based implementation 🤔 .
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501933746
##########
datafusion/expr/src/signature.rs:
##########
@@ -133,25 +136,168 @@ pub enum ArrayFunctionSignature {
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
ArrayAndIndex,
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_element_and_optional_index(
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ // make sure there's 2 or 3 arguments
+ if !(current_types.len() == 2 || current_types.len() == 3) {
+ return Ok(vec![vec![]]);
+ }
+
+ let first_two_types = ¤t_types[0..2];
+ let mut valid_types =
+ array_append_or_prepend_valid_types(first_two_types, true)?;
+
+ // Early return if there are only 2 arguments
+ if current_types.len() == 2 {
+ return Ok(valid_types);
+ }
+
+ let valid_types_with_index = valid_types
+ .iter()
+ .map(|t| {
+ let mut t = t.clone();
+ t.push(DataType::Int64);
+ t
+ })
+ .collect::<Vec<_>>();
+
+ valid_types.extend(valid_types_with_index);
+
+ Ok(valid_types)
+ }
+
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let new_array_type =
+ datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match new_array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let new_elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![new_array_type.clone(), new_elem_type.clone()]])
+ } else {
+ Ok(vec![vec![new_elem_type.to_owned(), new_array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+ let array_type = ¤t_types[0];
+ let array_type = array(&[array_type.clone()])?;
+ if array_type[0].is_empty() {
+ Ok(vec![vec![]])
+ } else {
+ Ok(vec![vec![array_type[0][0].clone(), DataType::Int64]])
+ }
+ }
+ fn array(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 1 {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ match array_type {
+ DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _) => {
+ let array_type = coerced_fixed_size_list_to_list(array_type);
+ Ok(vec![vec![array_type]])
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ match self {
+ ArrayFunctionSignature::ArrayAndElement => {
+ array_append_or_prepend_valid_types(current_types, true)
+ }
+ ArrayFunctionSignature::ElementAndArray => {
+ array_append_or_prepend_valid_types(current_types, false)
+ }
+ ArrayFunctionSignature::ArrayAndIndex => array_and_index(current_types),
+ ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
+ array_element_and_optional_index(current_types)
+ }
+ ArrayFunctionSignature::Array => array(current_types),
+ }
+ }
}
impl std::fmt::Display for ArrayFunctionSignature {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
match self {
ArrayFunctionSignature::ArrayAndElement => {
- write!(f, "array, element")
+ write!(f, "ArrayAndElement")
}
ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
write!(f, "array, element, [index]")
Review Comment:
forgot to change this?
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501935341
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -1874,6 +2038,17 @@ select
----
[4] [] [1, , 3, 4] [, , 1]
+#TODO: https://github.com/apache/arrow-datafusion/issues/9158
+#query ????
+#select
+# array_append(arrow_cast(make_array(), 'FixedSizeList(1, Null)'), 4),
Review Comment:
FixedSizeList(0) is equivalent to an empty list, so I think we can ignore it.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501348925
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
Maybe we can ignore nulls as the first argument since we are not coercing them to any other type. Unlike, array_append(arr, null) as the second argument, in this case, we will convert to other primitives.
`array()` for fixed list to list type coercion only.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501346257
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
I notice that in postgres, `array_append(null, 1)` return {1}, `array_append(null, ARRAY[1])` is error. 😕
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` , `array_positions`, `array_remove` and `array_remove_all`should use it.
```
D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
D SELECT list_position(null, 1);
┌────────────────────────┐
│ list_position(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
```
```
**Schema (PostgreSQL v15)**
---
**Query #1**
SELECT array_remove(NULL, 2);
| array_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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1493916770
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
`array_has` should use it.
```
D SELECT list_contains(null, 1);
┌────────────────────────┐
│ list_contains(NULL, 1) │
│ int32 │
├────────────────────────┤
│ │
└────────────────────────┘
```
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490691423
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let array_type = datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![array_type.clone(), elem_type.clone()]])
+ } else {
+ Ok(vec![vec![elem_type.to_owned(), array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
+ return Ok(vec![vec![]]);
+ }
+
+ match array_type {
+ DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _) => {
+ let array_type = coerced_fixed_size_list_to_list(array_type);
+ Ok(vec![vec![array_type, DataType::Int64]])
+ }
+ DataType::Null => Ok(vec![vec![array_type.clone(), DataType::Int64]]),
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array(
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 1
+ || (current_types[0].is_null() && !allow_null_coercion)
+ {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ match array_type {
+ DataType::List(_)
+ | DataType::LargeList(_)
+ | DataType::FixedSizeList(_, _) => {
+ let array_type = coerced_fixed_size_list_to_list(array_type);
+ Ok(vec![vec![array_type]])
+ }
+ DataType::Null => Ok(vec![vec![array_type.clone()]]),
+ _ => Ok(vec![vec![DataType::List(Arc::new(Field::new(
Review Comment:
I think we should return error if not List
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1475706777
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4252,21 +4252,22 @@ NULL [3] [4]
# array_ndims scalar function #1
-query III
+query error
+selrct array_ndims(1), array_ndims(null)
Review Comment:
follow PostgreSQL
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490621536
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
Review Comment:
why do we need `allow_null_coercion`?
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "Weijun-H (via GitHub)" <gi...@apache.org>.
Weijun-H commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1482354849
##########
datafusion/sqllogictest/test_files/array.slt:
##########
@@ -4262,21 +4337,25 @@ NULL [3] [4]
# array_ndims scalar function #1
-query III
+query error
+select array_ndims(1)
+
+query error
+select array_ndims(null)
+
Review Comment:
follow PostgreSQL
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1490695072
##########
datafusion/expr/src/signature.rs:
##########
@@ -123,29 +128,171 @@ pub enum TypeSignature {
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ArrayFunctionSignature {
/// Specialized Signature for ArrayAppend and similar functions
- /// The first argument should be List/LargeList, and the second argument should be non-list or list.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be non-list or list.
/// The second argument's list dimension should be one dimension less than the first argument's list dimension.
/// List dimension of the List/LargeList is equivalent to the number of List.
/// List dimension of the non-list is 0.
- ArrayAndElement,
+ ArrayAndElement(bool),
/// Specialized Signature for ArrayPrepend and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
/// The first argument should be non-list or list, and the second argument should be List/LargeList.
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
- ElementAndArray,
- ArrayAndIndex,
+ ElementAndArray(bool),
+ /// Specialized Signature for ArrayEqual and similar functions
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
+ ArrayAndIndex(bool),
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ /// If `allow_null` is true, the function also accepts a single argument of type Null.
+ Array(bool),
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// `allow_null_coercion` - Whether null type coercion is allowed
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let array_type = datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![array_type.clone(), elem_type.clone()]])
+ } else {
+ Ok(vec![vec![elem_type.to_owned(), array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(
+ current_types: &[DataType],
+ allow_null_coercion: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let array_type = ¤t_types[0];
+
+ if array_type.eq(&DataType::Null) && !allow_null_coercion {
+ return Ok(vec![vec![]]);
+ }
+
+ match array_type {
Review Comment:
Better to reuse `array`
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1499263824
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
We may need functions like `pre-checking`, or `validation` for checking the input of the Array function when we move to UDF-based implementation 🤔 .
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501348925
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
Maybe we can just ignore nulls as first argument since we are not coercing them to any other type. Unlike, array_append(arr, null) as second argument, in this case we will convert to other primitives.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501928036
##########
datafusion/expr/src/signature.rs:
##########
@@ -133,25 +136,168 @@ pub enum ArrayFunctionSignature {
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
ArrayAndIndex,
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_element_and_optional_index(
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ // make sure there's 2 or 3 arguments
+ if !(current_types.len() == 2 || current_types.len() == 3) {
+ return Ok(vec![vec![]]);
+ }
+
+ let first_two_types = ¤t_types[0..2];
+ let mut valid_types =
+ array_append_or_prepend_valid_types(first_two_types, true)?;
+
+ // Early return if there are only 2 arguments
+ if current_types.len() == 2 {
+ return Ok(valid_types);
+ }
+
+ let valid_types_with_index = valid_types
+ .iter()
+ .map(|t| {
+ let mut t = t.clone();
+ t.push(DataType::Int64);
+ t
+ })
+ .collect::<Vec<_>>();
+
+ valid_types.extend(valid_types_with_index);
+
+ Ok(valid_types)
+ }
+
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let new_array_type =
+ datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match new_array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let new_elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![new_array_type.clone(), new_elem_type.clone()]])
+ } else {
+ Ok(vec![vec![new_elem_type.to_owned(), new_array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
Review Comment:
```suggestion
fn array(array_type: &DataType) -> Option<DataType> {
match array_type {
DataType::List(_)
| DataType::LargeList(_)
| DataType::FixedSizeList(_, _) => {
let array_type = coerced_fixed_size_list_to_list(array_type);
Some(array_type)
}
_ => None,
}
}
match self {
ArrayFunctionSignature::ArrayAndElement => {
array_append_or_prepend_valid_types(current_types, true)
}
ArrayFunctionSignature::ElementAndArray => {
array_append_or_prepend_valid_types(current_types, false)
}
ArrayFunctionSignature::ArrayAndIndex => {
if current_types.len() != 2 {
return Ok(vec![vec![]]);
}
let array_type = array(¤t_types[0]);
if let Some(array_type) = array_type {
Ok(vec![vec![array_type, DataType::Int64]])
} else {
Ok(vec![])
}
}
ArrayFunctionSignature::ArrayAndElementAndOptionalIndex => {
array_element_and_optional_index(current_types)
}
ArrayFunctionSignature::Array => {
if current_types.len() != 1 {
return Ok(vec![vec![]]);
}
let array_type = array(¤t_types[0]);
if let Some(array_type) = array_type {
Ok(vec![vec![array_type]])
} else {
Ok(vec![])
}
}
}
```
##########
datafusion/expr/src/signature.rs:
##########
@@ -133,25 +136,168 @@ pub enum ArrayFunctionSignature {
/// The first argument's list dimension should be one dimension less than the second argument's list dimension.
ElementAndArray,
/// Specialized Signature for Array functions of the form (List/LargeList, Index)
+ /// The first argument should be List/LargeList/FixedSizedList, and the second argument should be Int64.
ArrayAndIndex,
/// Specialized Signature for Array functions of the form (List/LargeList, Element, Optional Index)
ArrayAndElementAndOptionalIndex,
+ /// Specialized Signature for ArrayEmpty and similar functions
+ /// The function takes a single argument that must be a List/LargeList/FixedSizeList
+ /// or something that can be coerced to one of those types.
+ Array,
+}
+
+impl ArrayFunctionSignature {
+ /// Arguments to ArrayFunctionSignature
+ /// `current_types` - The data types of the arguments
+ /// Returns the valid types for the function signature
+ pub fn get_type_signature(
+ &self,
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ fn array_element_and_optional_index(
+ current_types: &[DataType],
+ ) -> Result<Vec<Vec<DataType>>> {
+ // make sure there's 2 or 3 arguments
+ if !(current_types.len() == 2 || current_types.len() == 3) {
+ return Ok(vec![vec![]]);
+ }
+
+ let first_two_types = ¤t_types[0..2];
+ let mut valid_types =
+ array_append_or_prepend_valid_types(first_two_types, true)?;
+
+ // Early return if there are only 2 arguments
+ if current_types.len() == 2 {
+ return Ok(valid_types);
+ }
+
+ let valid_types_with_index = valid_types
+ .iter()
+ .map(|t| {
+ let mut t = t.clone();
+ t.push(DataType::Int64);
+ t
+ })
+ .collect::<Vec<_>>();
+
+ valid_types.extend(valid_types_with_index);
+
+ Ok(valid_types)
+ }
+
+ fn array_append_or_prepend_valid_types(
+ current_types: &[DataType],
+ is_append: bool,
+ ) -> Result<Vec<Vec<DataType>>> {
+ if current_types.len() != 2 {
+ return Ok(vec![vec![]]);
+ }
+
+ let (array_type, elem_type) = if is_append {
+ (¤t_types[0], ¤t_types[1])
+ } else {
+ (¤t_types[1], ¤t_types[0])
+ };
+
+ // We follow Postgres on `array_append(Null, T)`, which is not valid.
+ if array_type.eq(&DataType::Null) {
+ return Ok(vec![vec![]]);
+ }
+
+ // We need to find the coerced base type, mainly for cases like:
+ // `array_append(List(null), i64)` -> `List(i64)`
+ let array_base_type = datafusion_common::utils::base_type(array_type);
+ let elem_base_type = datafusion_common::utils::base_type(elem_type);
+ let new_base_type = comparison_coercion(&array_base_type, &elem_base_type);
+
+ let new_base_type = new_base_type.ok_or_else(|| {
+ internal_datafusion_err!(
+ "Coercion from {array_base_type:?} to {elem_base_type:?} not supported."
+ )
+ })?;
+
+ let new_array_type =
+ datafusion_common::utils::coerced_type_with_base_type_only(
+ array_type,
+ &new_base_type,
+ );
+
+ match new_array_type {
+ DataType::List(ref field)
+ | DataType::LargeList(ref field)
+ | DataType::FixedSizeList(ref field, _) => {
+ let new_elem_type = field.data_type();
+ if is_append {
+ Ok(vec![vec![new_array_type.clone(), new_elem_type.clone()]])
+ } else {
+ Ok(vec![vec![new_elem_type.to_owned(), new_array_type.clone()]])
+ }
+ }
+ _ => Ok(vec![vec![]]),
+ }
+ }
+ fn array_and_index(current_types: &[DataType]) -> Result<Vec<Vec<DataType>>> {
Review Comment:
I prefer to return the data type for `array` instead of long but useless `Result<Vec<Vec<DataType>>>`
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108#discussion_r1501352016
##########
datafusion/physical-expr/src/array_expressions.rs:
##########
@@ -433,6 +435,7 @@ pub fn array_element(args: &[ArrayRef]) -> Result<ArrayRef> {
let indexes = as_int64_array(&args[1])?;
general_array_element::<i64>(array, indexes)
}
+ DataType::Null => Ok(args[0].clone()),
Review Comment:
I believe our primary focus within TypeCoercionRewriter should be strictly on type coercion. If encountering a null value that isn't being cast to another type, it should be disregarded.
--
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] feat: support `FixedSizeList` Type Coercion [arrow-datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #9108:
URL: https://github.com/apache/arrow-datafusion/pull/9108
--
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