-
Notifications
You must be signed in to change notification settings - Fork 177
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
Impose FilterableValue
in UniqueColumn::find
#2267
Impose FilterableValue
in UniqueColumn::find
#2267
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this is what was intended by the ticket, but if it is, this LGTM.
(Which ticket is this tied to?) |
ce8e61f
to
370ec59
Compare
370ec59
to
2456bcb
Compare
I think you need to be sure in order to approve the PR. |
FTR: Mazdak said that @cloutiertyler said that this PR implements the intended behavior. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love the test!
Description of Changes
Fixes #2272.
This PR does the following:
UniqueIndex::find
must beFilterableValue
.FilterableValue
a sealed trait so that we may intentionally grow the set and make it un-sealed at a time of our choosing.#[diagnostic::on_unimplemented(...)]
toFilterableValue
for better diagnostics..filter(..)
and.find(...)
imposeFilterableValue
.The PR does not impose the constraint on table columns themselves.
Perhaps that is what we want to do, as it would allow us to also impose the check on the semantic schema level in the host.
This would be more foolproof.
API and ABI breaking changes
Yes but its a bug fix.
Expected complexity level and risk
1
Testing
A UI test has been added.