alamb commented on code in PR #7885:
URL: https://github.com/apache/arrow-datafusion/pull/7885#discussion_r1367138171
##########
datafusion/physical-expr/src/intervals/interval_aritmetic.rs:
##########
@@ -235,18 +247,16 @@ impl Display for Interval {
impl Interval {
/// Creates a new interval object using the given bounds.
///
- /// # Boolean intervals need special handling
+ /// As explained in [`Interval]` boolean `Interval`s are special and this
Review Comment:
Given that there are only three valid boolean intervals, and
`Interval::new()` normalizes to one of those, it might make sense to make the
fields of `Interval` non `pub` so that it is not possible to construct an
invalid Interval
##########
datafusion/physical-expr/src/intervals/interval_aritmetic.rs:
##########
@@ -1079,6 +1089,117 @@ mod tests {
Interval::make(lower, upper, (false, false))
}
+ #[test]
+ fn boolean_interval_test() -> Result<()> {
+ // Demonstrate / document how boolean Intervals work
+ #[derive(Debug)]
+ struct TestCase {
+ lower: bool,
+ upper: bool,
+ /// expected bounds when lower is open, upper is open
+ expected_open_open: (bool, bool),
+ /// expected bounds when lower is open, upper is closed
+ expected_open_closed: (bool, bool),
+ /// expected bounds when lower is closes, upper is open
+ expected_closed_open: (bool, bool),
+ // Not: closed/closed is the same as lower/upper
+ }
+
+ let cases = vec![
Review Comment:
I was really confused about what the expected Intervals were, so I added
test cases. it would be great if someone could double check these
##########
datafusion/physical-expr/src/intervals/interval_aritmetic.rs:
##########
@@ -194,6 +194,18 @@ impl Display for IntervalBound {
/// called *unbounded* endpoint and represented using a `NULL` and written
using
/// `∞`.
///
+/// # Boolean Handling
+///
+/// Boolean values require special handling.
+///
+/// Given there are only two boolean values, and they are ordered such that
+/// `false` is less than `true`, there are only three possible valid intervals
+/// for a boolean `[false, false]`, `[false, true]` or `[true, true]`, all with
Review Comment:
The core of my confusion is that boolean `Intervals` NEVER have open bounds
-- and if you try to make one with open bounds, `Interval::new` will remap them
--
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]
