Skip to content

WIP Support NULL literals in Binary expressions, add some missing LargeStringArray implementations - #1181

Closed
alamb wants to merge 3 commits into
apache:masterfrom
alamb:alamb/comparisons_to_null
Closed

alamb wants to merge 3 commits into
apache:masterfrom
alamb:alamb/comparisons_to_null

Conversation

@alamb

@alamb alamb commented Oct 27, 2021 •

Copy link
Copy Markdown
Contributor

NOT YET WORKING / READY FOR REVIEW

I thought this was ready but when adding new tests it is not. Thus leaving it as a draft, but still needs more work

Which issue does this PR close?

Fixes #1179

Rationale for this change

Binary operations to literal NULL don't work in DataFusion. While seemingly a corner case, we sometimes generate such predicates during rewrites in IOx making supporting such expressions important.

What changes are included in this PR?

  1. Add special cast handling for null ScalarValues
  2. Add tests

Note this is far from a comprehensive solution -- it is a localized fix for binary expression evaluation. The more comprehsive fix is discussed by @Dandandan and @jimexist on #1179 -- see #1179 (comment)

However, I think this PR should be merged because:

  1. includes additional test coverage that will be helpful when we do the more comprehensive fix and it
  2. It selfishly helps me in IOx for the short term

Are there any user-facing changes?

Fewer errors, no breaking changes

Follow on work

  • TODO File arrow-rs ticket about underflow when subtracting from null array
  • TODO File tickets for supporting null constants in other exprs (like in function call parameters, CASE, etc)
  • TODO file tickets for casting from NUll to other types in arrow

@alamb alamb changed the title Fix comparisons to NULL literal, add some missing LargeStringArray implementations Support NULL literals in Binary expressions, add some missing LargeStringArray implementations Oct 27, 2021
}
}

/// if value represents a null literal (`ScalarValue::*(None)`) return

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Technically a Null literal can be cast to any other type. It would be cool to properly supportDataType::Null throughout DataFusion (I spent a while trying to do this, but found many places that are not prepared to handle it -- like arrow cast kernel)

@alamb alamb changed the title Support NULL literals in Binary expressions, add some missing LargeStringArray implementations WIP Support NULL literals in Binary expressions, add some missing LargeStringArray implementations Oct 27, 2021
}

#[test]
fn test_compare_to_null_literal() {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This test found lots of good stuff -- I would say it is the best part of this PR at the moment

@jimexist

jimexist commented Nov 8, 2021

Copy link
Copy Markdown
Member

related #1257

@alamb

alamb commented Dec 29, 2021

Copy link
Copy Markdown
Contributor Author

Closing as I haven't had a chance to work on it for a while

@alamb alamb closed this Dec 29, 2021
@alamb
alamb deleted the alamb/comparisons_to_null branch August 8, 2023 20:11
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.

Comparisons (BinaryExprs) to literal NULL don't work

2 participants