Skip to content

Translate COALESCE as ISNULL in more cases #35986

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Apr 23, 2025

This is a continuation of #34171 that extends the conversion to handle all of the cases (and adds a couple of tests for corner-cases).

@ranma42 ranma42 force-pushed the isnull-more branch 2 times, most recently from b688d03 to d6708d9 Compare April 27, 2025 19:05
@ranma42 ranma42 force-pushed the isnull-more branch 2 times, most recently from 5b4b67b to 4f4a019 Compare April 28, 2025 06:12
@ranma42
Copy link
Contributor Author

ranma42 commented May 7, 2025

Some additional notes: I tried to implement all of the suggestions by @roji and indeed they proved quite effective; OTOH the tests I added unveiled some additional limitations risks, which I mitigated with an approach the codebase is already taking elsewhere (basically widening the type).
I think it should be reasonable, but this might both be:

  • insufficient in some cases (I only implemented the widening for 3 SQL types; I am unsure if other types might need a similar approach)
  • it will result in different (SQL) types for the expressions

Specifically the changes I implemented include:

  1. rely on StoreType to check if the types match
  2. preserve the computed nullability
  3. handle most of the types

The conversion now applies to all instances of COALESCE. Some limitation are:

  • in several cases, unneeded Convert operations are inserted; in the case of sized types ([n]varchar, varbinary) they are almost always forced
  • nullability is only propagated, not re-computed (I think if we want to have a relevant value here, we should compute it in the during the nullability processing and propagate it from there; this change is mostly to make sure the if/when we do that, the result is automatically forwarded as needed)

Even though it passes the testsuite, I think it might be a good idea to make it possible for users to opt-out, as I am afraid it might break some corner cases (especially around UDF and/or custom converters).

@ranma42 ranma42 marked this pull request as ready for review May 7, 2025 12:36
@ranma42 ranma42 requested a review from a team as a code owner May 7, 2025 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants