Skip to content

Conversation

@pepijnve
Copy link
Contributor

Which issue does this PR close?

None, unit test clarification

Rationale for this change

The unit tests for Interval::and, and Interval::not are written using a hard to to interpret matrix of boolean values. It's much easier to scan this for correctness when using the constants instead.

What changes are included in this PR?

  • Replace raw boolean values with constant references
  • Ensure tests cover all permutations
  • Add missing test for Interval::or

Are these changes tested?

Test only change

Are there any user-facing changes?

No

@github-actions github-actions bot added the logical-expr Logical plan and expressions label Nov 11, 2025
@pepijnve pepijnve changed the title Clarify Interval 'and', 'or', and 'not' tests Clarify tests for Interval::and, 'Interval::not, and add Interval::or` tests Nov 11, 2025
@pepijnve pepijnve changed the title Clarify tests for Interval::and, 'Interval::not, and add Interval::or` tests Clarify tests for Interval::and, Interval::not, and add Interval::or tests Nov 11, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @pepijnve -- I agree this is much easier to understand

(false, true, true, true, false, true),
(false, false, false, false, false, false),
(true, true, true, true, true, true),
(
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 -- this is much easier to understand

Maybe we could also add a comment explaining what each field is -- <arg0> AND <arg1> and expected output <arg0>

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

I see #18619 outstanding so let's merge this one in to avoid unecessary conflicts

@alamb alamb added this pull request to the merge queue Nov 11, 2025
@alamb alamb added the development-process Related to development process of DataFusion label Nov 11, 2025
Merged via the queue into apache:main with commit 2066568 Nov 11, 2025
34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

development-process Related to development process of DataFusion logical-expr Logical plan and expressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants