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

[expr] Add a proptest for the is_monotone annotations #29538

Merged
merged 2 commits into from
Sep 17, 2024

Conversation

bkirwi
Copy link
Contributor

@bkirwi bkirwi commented Sep 13, 2024

Motivation

Why think of counterexamples when the computer can do it?

(We could also add more functions to the abstract interpreter tests, which cover this codepath... but we can test this directly more cheaply for more functions, and this keeps any test failures closer to the problematic code.)

Tips for reviewer

Inspired by: #29514

This sadly produces a counterexample for left when passed a negative argument... 'a' < 'aa' < 'z' but left(_, -1) returns '' < 'a' > ''. Concat seems fine in the second arg however!

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@bkirwi bkirwi requested a review from a team as a code owner September 13, 2024 19:07
@bkirwi bkirwi requested a review from ggevay September 13, 2024 19:51
The original reason this was banned doesn't apply here, and we can't use
the suggested alternative for lifetimey reasons.
Copy link
Contributor

@ggevay ggevay left a comment

Choose a reason for hiding this comment

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

This is great, thank you! Monotonicity can be indeed really tricky sometimes!

I think we could merge this as it is, and then make it more comprehensive later when we have time:

  • Could make this a part of test_smoketest_all_builtins.
  • Could use arb_datum_for_column in addition to interesting_datums.
  • Could make it test could_error, preserves_uniqueness, inverse.

I've created a follow-up issue: https://github.com/MaterializeInc/materialize/issues/29584

@ggevay
Copy link
Contributor

ggevay commented Sep 17, 2024

I've adjusted #29514 and brought it out of draft.

@bkirwi
Copy link
Contributor Author

bkirwi commented Sep 17, 2024

I've created a follow-up issue [...]

Great! Definitely support integrating into whatever feels most natural for you. Thanks for the review!

@bkirwi bkirwi merged commit ab17de9 into MaterializeInc:main Sep 17, 2024
81 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants