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

fix(core): if-then expression should be nullable if any 'then' is nullable #287

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

bvolpato
Copy link
Member

Recently had some nullability issues with Calcite coming from the SQL:

  CASE WHEN record_type = 'abc' THEN
    field_a
  ELSE
    ''
  END

Because the else part is a literal, nullable was false, but it wasn't actually true, as field_a is in fact nullable.

Calcite would fail assertions because of this mismatch:

rpc error: code = Internal desc = Cannot add expression of different type to set:
set type is RecordType(VARCHAR NOT NULL $f30, VARCHAR $f10) NOT NULL
expression type is RecordType(VARCHAR $f3, VARCHAR $f1) NOT NULL
set is org.apache.calcite.plan.volcano.RelSet@6624a103
expression is LogicalSort(sort0=[$1], dir0=[DESC], fetch=[5])
...
Type mismatch:
rowtype of original rel: RecordType(VARCHAR NOT NULL $f30, VARCHAR $f10) NOT NULL
rowtype of new rel: RecordType(VARCHAR $f3, VARCHAR $f1) NOT NULL
Difference:
$f30: VARCHAR NOT NULL -> VARCHAR

Simply allowing the case to be nullable fixes the issues for our query, so sending that upstream since I think it makes sense and it was just overlooked before.

Copy link
Member

@vbarua vbarua left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks for fixing.

@vbarua vbarua merged commit a0ca17b into substrait-io:main Aug 16, 2024
12 checks passed
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.

2 participants