alamb commented on code in PR #18625:
URL: https://github.com/apache/datafusion/pull/18625#discussion_r2515425735
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1756,27 +1794,31 @@ impl NullableInterval {
/// Return true if the value is definitely true (and not null).
pub fn is_certainly_true(&self) -> bool {
- match self {
- Self::Null { .. } | Self::MaybeNull { .. } => false,
- Self::NotNull { values } => values == &Interval::CERTAINLY_TRUE,
- }
+ self == &Self::TRUE
Review Comment:
while your new implementations are simpler, I think they are equivalent to
the old ones
Or am I missing something here?
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1738,6 +1738,44 @@ impl From<ScalarValue> for NullableInterval {
}
impl NullableInterval {
+ /// An interval that only contains 'false'.
+ pub const FALSE: Self = NullableInterval::NotNull {
+ values: Interval::CERTAINLY_FALSE,
+ };
+
+ /// An interval that only contains 'true'.
+ pub const TRUE: Self = NullableInterval::NotNull {
+ values: Interval::CERTAINLY_TRUE,
+ };
+
+ /// An interval that only contains 'unknown' (boolean null).
+ pub const UNKNOWN: Self = NullableInterval::Null {
+ datatype: DataType::Boolean,
+ };
+
+ /// An interval that only contains 'true' and 'false'.
+ /// This interval is equivalent to [Interval::UNCERTAIN].
+ pub const TRUE_OR_FALSE: Self = NullableInterval::NotNull {
+ values: Interval::UNCERTAIN,
+ };
+
+ /// An interval that only contains 'true' and 'unknown'.
+ pub const TRUE_OR_UNKNOWN: Self = NullableInterval::MaybeNull {
+ values: Interval::CERTAINLY_TRUE,
+ };
+
+ /// An interval that only contains 'false' and 'unknown'.
+ pub const FALSE_OR_UNKNOWN: Self = NullableInterval::MaybeNull {
+ values: Interval::CERTAINLY_FALSE,
+ };
+
+ /// An interval that contains all possible boolean values: 'true', 'false'
and 'unknown'.
+ ///
+ /// Note that this is different from [Interval::UNCERTAIN] which only
contains 'true' and 'false'.
+ pub const UNCERTAIN: Self = NullableInterval::MaybeNull {
Review Comment:
I personally found it hard to keep track of how UNCERTAIN differed from
UNKNOWN and the other enums
Maybe we could name it something like `TRUE_OR_FALSE_OR_NULL` (even though
that is a mouthful it is the most explicit)
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -1738,6 +1738,44 @@ impl From<ScalarValue> for NullableInterval {
}
impl NullableInterval {
+ /// An interval that only contains 'false'.
+ pub const FALSE: Self = NullableInterval::NotNull {
+ values: Interval::CERTAINLY_FALSE,
+ };
+
+ /// An interval that only contains 'true'.
+ pub const TRUE: Self = NullableInterval::NotNull {
+ values: Interval::CERTAINLY_TRUE,
+ };
+
+ /// An interval that only contains 'unknown' (boolean null).
+ pub const UNKNOWN: Self = NullableInterval::Null {
Review Comment:
I found this wording confusing (as I do much of this tri-state logic)
because we "know" something about this interval -- that it is null. So it is a
known unknown 🤔
Maybe we could call this `NULL` or `ONLY_NULL` or something to make it less
confusing
##########
datafusion/expr-common/src/interval_arithmetic.rs:
##########
@@ -4226,4 +4327,555 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn nullable_and_test() -> Result<()> {
+ let cases = vec![
+ (
+ NullableInterval::TRUE,
+ NullableInterval::TRUE,
+ NullableInterval::TRUE,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::FALSE,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::UNKNOWN,
+ NullableInterval::UNKNOWN,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::TRUE_OR_FALSE,
+ NullableInterval::TRUE_OR_FALSE,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::TRUE_OR_UNKNOWN,
+ NullableInterval::TRUE_OR_UNKNOWN,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::FALSE_OR_UNKNOWN,
+ NullableInterval::FALSE_OR_UNKNOWN,
+ ),
+ (
+ NullableInterval::TRUE,
+ NullableInterval::UNCERTAIN,
+ NullableInterval::UNCERTAIN,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::TRUE,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::FALSE,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::UNKNOWN,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::TRUE_OR_FALSE,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::TRUE_OR_UNKNOWN,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::FALSE_OR_UNKNOWN,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::FALSE,
+ NullableInterval::UNCERTAIN,
+ NullableInterval::FALSE,
+ ),
+ (
+ NullableInterval::UNKNOWN,
+ NullableInterval::TRUE,
+ NullableInterval::UNKNOWN,
+ ),
+ (
+ NullableInterval::UNKNOWN,
+ NullableInterval::FALSE,
+ NullableInterval::FALSE,
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
