pepijnve commented on code in PR #18625:
URL: https://github.com/apache/datafusion/pull/18625#discussion_r2518641184
##########
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:
@alamb I was wondering if we might want to consider renaming the Interval
constants as well. I think this mapping would be nicer since they match exactly
with their `NullableInterval` equivalents then in both name and semantics.
- `Interval::CERTAINLY_TRUE` -> `Interval::TRUE`
- `Interval::CERTAINLY_FALSE` -> `Interval::FALSE`
- `Interval::UNCERTAIN` -> `Interval::TRUE_OR_FALSE`
For `NullableInterval::UNCERTAIN` I'm inclined to go for `ANY_TRUTH_VALUE`
rather than `TRUE_OR_FALSE_OR_UNKNOWN` just to keep it somewhat short.
For backwards compatibility it might be useful to retain the old constants
and mark them deprecated. Not sure what the DataFusion policy on stuff like
that is.
##########
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:
@alamb I was wondering if we might want to consider renaming the `Interval`
constants. I think this mapping would be nicer since they match exactly with
their `NullableInterval` equivalents then in both name and semantics.
- `Interval::CERTAINLY_TRUE` -> `Interval::TRUE`
- `Interval::CERTAINLY_FALSE` -> `Interval::FALSE`
- `Interval::UNCERTAIN` -> `Interval::TRUE_OR_FALSE`
For `NullableInterval::UNCERTAIN` I'm inclined to go for `ANY_TRUTH_VALUE`
rather than `TRUE_OR_FALSE_OR_UNKNOWN` just to keep it somewhat short.
For backwards compatibility it might be useful to retain the old constants
and mark them deprecated. Not sure what the DataFusion policy on stuff like
that is.
##########
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 see you've already gone ahead and merged, so I just missed it, but for
consistency see
https://github.com/apache/datafusion/pull/18625#discussion_r2518641184
I can make a followup for that if you like.
--
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]
