Conversation
| let expected = [ | ||
| "+--------------+", | ||
| "| result |", | ||
| "+--------------+", | ||
| "| |", | ||
| "| {a: 2, b: 2} |", | ||
| "| |", | ||
| "+--------------+", | ||
| ]; | ||
|
|
||
| assert_batches_eq!(expected, &[batch]); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
I'd try to be consistent with the other tests in this file and, rather than asserting a string representation of the record batches, I'd try to perform assertions of the returned ArrayRefs.
If you think it's strictly necessary to perform assertions over pretty-printed string representations, this project typically uses insta for managing them.
| datafusion-expr = { workspace = true } | ||
| datafusion-expr-common = { workspace = true } | ||
| datafusion-macros = { workspace = true } | ||
| datafusion-physical-expr-common = { workspace = true } |
There was a problem hiding this comment.
🤔 I don't see a problem in introducing this coupling point here, but I'll double check if this is fine.
There was a problem hiding this comment.
This should be fine. Other function-* crates in this project are already depending on this module, for example:
https://github.com/apache/datafusion/blob/branch-53/datafusion/functions-nested/Cargo.toml#L59-L59
| #[test] | ||
| fn nullif_struct() -> Result<()> { |
There was a problem hiding this comment.
Can we add a new test comparing a nested array VS a non-nested array? I imagine the result of comparing both should return false, but I'm not sure if right now that would happen, as we might hit this line here:
2 participants
Which issue does this PR close?
nullif. #21763Rationale for this change
Add support for nested types to the
nullifUDF.Are these changes tested?
Unit tests included.
Are there any user-facing changes?
No changes to the function's signature.