Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor: Fix bug in AND interval analysis tests (not code), and add more coverage #7886

Closed
wants to merge 1 commit into from

Conversation

alamb
Copy link
Contributor

@alamb alamb commented Oct 20, 2023

Which issue does this PR close?

Related to https://github.com/apache/arrow-datafusion/pull/7884/files

Rationale for this change

When I was figuring out how to add OR support for interval analysis, I noticed:

  1. The tests were using open_open, which as shown on Minor: Clarify Boolean Interval handling and verify it with a test #7885 means all the cases actually were testing the same
  2. There were three cases that are not covered.

What changes are included in this PR?

  1. Fix the test to use closed_closed
  2. Add coverage of missing cases

Are these changes tested?

Are there any user-facing changes?

@alamb alamb changed the title Minor: Fix bug in AND interval analysis, and add more coverage Minor: Fix bug in AND interval analysis tests (not code), and add more coverage Oct 20, 2023
@github-actions github-actions bot added the physical-expr Physical Expressions label Oct 20, 2023
(false, true, true, true, false, true),
(false, false, false, false, false, false),
(true, true, true, true, true, true),
// Note there are only three valid boolean intervals: [false, false], [false, true] and [true, true],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the three missing cases and reordered the cases so it was easier to see what is covered and what was not.

@alamb
Copy link
Contributor Author

alamb commented Oct 23, 2023

Marking as a draft until @berkaysynnada 's refactor PR has landed

@alamb alamb marked this pull request as draft October 23, 2023 13:19
@alamb
Copy link
Contributor Author

alamb commented Nov 20, 2023

Superseded by #8276 (the whole notion of open intervals has been removed)

@alamb alamb closed this Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant