feature: support nvl(ifnull) function #9284
Conversation
Co-authored-by: Jonah Gao <jonahgao@msn.com>
datafusion/functions/src/core/nvl.rs
Outdated
| let size = lhs.len(); | ||
| let mut current_value = rhs.to_array_of_size(size)?; | ||
| let remainder = BooleanArray::from(vec![true; size]); | ||
| let to_apply = and(&remainder, &is_not_null(lhs.as_ref())?)?; |
There was a problem hiding this comment.
Is this "and" operation necessary, as the remainder is always true ?
There was a problem hiding this comment.
ohh, yes, "and" true is meaningless, thank you 👍
datafusion/functions/src/core/nvl.rs
Outdated
| /// Currently supported types by the nvl/ifnull function. | ||
| /// The order of these types correspond to the order on which coercion applies | ||
| /// This should thus be from least informative to most informative | ||
| pub static SUPPORTED_NVL_TYPES: &[DataType] = &[ |
There was a problem hiding this comment.
| pub static SUPPORTED_NVL_TYPES: &[DataType] = &[ | |
| static SUPPORTED_NVL_TYPES: &[DataType] = &[ |
How about removing pub since it is only used within the current file?
| let current_value = zip(&to_apply, lhs, &rhs)?; | ||
| Ok(ColumnarValue::Array(current_value)) | ||
| } | ||
| (ColumnarValue::Scalar(lhs), ColumnarValue::Array(rhs)) => { |
There was a problem hiding this comment.
How about merging the computation related to arrays together?
Cloning an ArrayRef should be cheap.
let (lhs_array, rhs_array) = match (&args[0], &args[1]) {
(ColumnarValue::Array(lhs), ColumnarValue::Scalar(rhs)) => {
(lhs.clone(), rhs.to_array_of_size(lhs.len())?)
}
(ColumnarValue::Array(lhs), ColumnarValue::Array(rhs)) => {
(lhs.clone(), rhs.clone())
}
(ColumnarValue::Scalar(lhs), ColumnarValue::Array(rhs)) => {
(lhs.to_array_of_size(rhs.len())?, rhs.clone())
}
(ColumnarValue::Scalar(lhs), ColumnarValue::Scalar(rhs)) => {
let mut current_value = lhs;
if lhs.is_null() {
current_value = rhs;
}
return Ok(ColumnarValue::Scalar(current_value.clone()));
}
};
let to_apply = is_not_null(&lhs_array)?;
let value = zip(&to_apply, &lhs_array, &rhs_array)?;
Ok(ColumnarValue::Array(value))There was a problem hiding this comment.
This looks better than mine
thank you @jonahgao 😄 |
| @@ -569,6 +569,7 @@ trunc(numeric_expression[, decimal_places]) | |||
|
|
|||
| - [coalesce](#coalesce) | |||
| - [nullif](#nullif) | |||
| - [nvl/ifnull](#nvl/ifnull) | |||
There was a problem hiding this comment.
This link to nvl/ifnull seems broken.
https://github.com/apache/arrow-datafusion/blob/f6c6e385041974c4e501bbe9e3ab42284ddddfda/docs/source/user-guide/sql/scalar_functions.md#conditional-functions
[nvl/ifnull](#nvlifnull) should work.
The rest of this PR looks good to me. Good job 👍
There was a problem hiding this comment.
not support /, I separated them， thank you again 😄
github-actions
bot
added
sql
github-actions
bot
removed
sql
|
|
||
| fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { | ||
| // NVL has two args and they might get coerced, get a preview of this | ||
| let coerced_types = datafusion_expr::type_coercion::functions::data_types(arg_types, &self.signature); |
There was a problem hiding this comment.
I don't fully understand the need for this code to check coercion, but I know it follows the existing patterns elsewhere.
Maybe we can try to remove it in a follow on PR
There was a problem hiding this comment.
#9357 for anyone else following along
| @@ -569,6 +569,8 @@ trunc(numeric_expression[, decimal_places]) | |||
|
|
|||
| - [coalesce](#coalesce) | |||
| - [nullif](#nullif) | |||
| - [nvl](#nvl) | |||
| - [ifnull](#ifnull) | |||
Which issue does this PR close?
Closes #9261.
Rationale for this change
add a new function
nvlWhat changes are included in this PR?
Are these changes tested?
yes
Are there any user-facing changes?
yes