Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add COALESCE function #4829

Merged
merged 6 commits into from
Mar 23, 2020

Conversation

big-andy-coates
Copy link
Contributor

@big-andy-coates big-andy-coates commented Mar 19, 2020

Description

The COALESCE function takes one or more parameters and returns the first that is non-null.

All parameters must be of the same type.

This function supersedes the existing IFNULL function, which only worked for strings and one accepted two parameters.

Note: also added docs for the ARRAY_LENGH I added recently, as I seemed to of missed adding docs!

Testing done

QTT + unit tests added.

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

The `COALESCE` function takes one or more parameters and returns the first that is non-null.

All parameters must be of the same type.

This function supersedes the existing `IFNULL` function, which only worked for strings and one accepted two parameters.
@big-andy-coates big-andy-coates requested review from JimGalasyn and a team as code owners March 19, 2020 16:49
@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented Mar 19, 2020

@blueedgenick @rmoff @derekjn @MichaelDrogalis: Just checking from an SQL / Product view that we're cool with this going in and deprecating ISNULL, (which only supports two STRING params.

@rmoff
Copy link
Member

rmoff commented Mar 19, 2020

LGTM, thanks Andy!

@big-andy-coates
Copy link
Contributor Author

big-andy-coates commented Mar 19, 2020

Raised #4831 to track the removal of the deprecated IFNULL pre version 1.0.

Alternatively, we could upgrade IFNULL to support any type and keep it, but it is basically a subset of COALESCE...

@derekjn
Copy link
Contributor

derekjn commented Mar 19, 2020

LGTM, solid addition 👍

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the new content!

@blueedgenick
Copy link
Contributor

Thanks @big-andy-coates , appreciate you soliciting the input. The new COALESCE looks good to me, could benefit from describing in the docs it's behavior with the "non-traditional" i.e. complex datatypes (map, array, struct) - e.g. is a struct containing a null considered to be a null value or not for the purposes of this comparison ? (i'd assume and hope that it's not, but worth calling out and maybe adding tests for). Likewise for an array with a single null entry and so on. These are the situations that may be unfamiliar to a user who is otherwise experienced with using a COALESCE function.

Not keen on removing the IfNull though, i'd much prefer to fix it to handle all types. I know it looks like a subset of COALESCE but it's common in some SQL dialects where COALESCE is not present - it's ok i think to have slightly overlapping functions if they help ease the mental model for users coming from different flavors of RDBMS background. At least MySQL and Vertica, by way of example, have both functions. SQLServer has ISNULL and COALESCE, etc.

One subtle difference to notice actually is that in many of the common DBMS, COALESCE is actually sugar for a CASE WHEN....CASE WHEN... construct. This is (slightly) significant because it's a "searched case" - meaning that each argument is only evaluated if needed, i.e. if the preceding ones turned out to be null. IFNULL, by contrast, always evaluates both input expressions. Our COALESCE implementation works in a more naive way, having first evaluated all of the expressions, put them in an array, and then passing to the UDF. This is slightly less efficient, especially if the parameters are themselves nested function calls. Not sure if we care about that just yet but it's worth noting for later.

@big-andy-coates
Copy link
Contributor Author

@blueedgenick

I've removed the deprecation, updated the docs and converted #4831 into a task to enhance IFNULL to support more than just STRING args.

@derekjn
Copy link
Contributor

derekjn commented Mar 19, 2020

All good points @blueedgenick, thanks for chiming in 👍

@blueedgenick
Copy link
Contributor

@blueedgenick

I've removed the deprecation, updated the docs and converted #4831 into a task to enhance IFNULL to support more than just STRING args.

Thanks! LGTM

Moved to separate PR.
@big-andy-coates big-andy-coates merged commit 251c237 into confluentinc:master Mar 23, 2020
@big-andy-coates big-andy-coates deleted the coalesce branch March 23, 2020 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants