feat: support nvl2 function #9364
Conversation
|
if this pr is merged, we can rewrite 🤔 |
| /// 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 | ||
| static SUPPORTED_NVL2_TYPES: &[DataType] = &[ |
There was a problem hiding this comment.
I don't think we need this list of data types, do we? Why wouldn't NVL2 support any data type? Its logic isn't specific to the type (only if the argument is null).
It seems like we could remove the list from nvl as well
There was a problem hiding this comment.
maybe Signature::variadic_equal(Volatility::Immutable) more suitable? don't need specific type 🤔
| use datafusion_common::{Result, ScalarValue}; | ||
|
|
||
| #[test] | ||
| fn nvl2_int32() -> Result<()> { |
There was a problem hiding this comment.
Do these tests add any additional coverage over what is in the nvl2.slt file? I feel like they are the same test logic encoded twice (as in it seems like any bug would cause both the slt and the rust tests to fail)
If they are the same, I think we should remove the rust based tests and stay with slt
There was a problem hiding this comment.
yes, This is indeed two similar tests, I will remove rust based tests
| ; | ||
|
|
||
| # Arrays tests | ||
| query I |
There was a problem hiding this comment.
I would recommend adding an ORDER BY to these queries to ensure the order is the same as listed in the tests
for example
SELECT NVL2(int_field, 2, 3) FROM test ORDER BY more_ints;
I think this would be a great idea. |
|
Thanks again @guojidan |
Which issue does this PR close?
Closes #9363.
Rationale for this change
What changes are included in this PR?
add nvl2 function
Are these changes tested?
Are there any user-facing changes?