Skip to content

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Sep 11, 2023

Which issue does this PR close?

related to #7467

Rationale for this change

While reviewing #7467 I found I had to remind myself several times what open/closed meant in the context of intervals.

What changes are included in this PR?

  1. Clarify comments on Interval (they were already nice, just could be somewhat clearer)
  2. Add Interval::new_open and Interval::new_closed as a way to make the code more self documenting
  3. Change some uses of Interval::new to Interval::new_open and Interval::new_closed

I am happy to change more uses but I wanted to see what people thought of the idea first

Are these changes tested?

Existing tests

Are there any user-facing changes?

No functional change intended, just easier to understand code

@github-actions github-actions bot added the physical-expr Changes to the physical-expr crates label Sep 11, 2023
@wjones127
Copy link
Member

wjones127 commented Sep 11, 2023

These seems good. I would also add: change get_datatype to just datatype to be more idiomatic. (Nevermind, didn't realize get_datatype() returned an owned DataType.)

The other change I was wondering about is whether Display could be implemented to output the set notation representation. Would be cool to get something like [10, 20], [10, ∞), (-∞, 100] or (-∞, ∞) back. But if we don't feel like that's idiomatic Rust, we should at least change the representation so it indicates whether the bounds are open or closed. Right now the Display implementation for IntervalBound just uses value.

@ozankabak
Copy link
Contributor

This looks good to me. I also like the display suggestion @wjones127 👍

@alamb
Copy link
Contributor Author

alamb commented Sep 13, 2023

The other change I was wondering about is whether Display could be implemented to output the set notation representation. Would be cool to get something like [10, 20], [10, ∞), (-∞, 100] or (-∞, ∞) back. But if we don't feel like that's idiomatic Rust, we should at least change the representation so it indicates whether the bounds are open or closed. Right now the Display implementation for IntervalBound just uses value.

I have filed #7547 to track this suggestion

@ozankabak and @wjones127 if you think this PR is ok to merge as is, can you please github approve it? If not, can you explain what else you would like to see?

Thank you!

Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@alamb alamb merged commit 8e2a0e6 into apache:main Sep 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants