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

Impose FilterableValue in UniqueColumn::find #2267

Merged
merged 2 commits into from
Feb 18, 2025

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Feb 17, 2025

Description of Changes

Fixes #2272.

This PR does the following:

  1. Imposes that the sought value column value for UniqueIndex::find must be FilterableValue.
  2. Makes FilterableValue a sealed trait so that we may intentionally grow the set and make it un-sealed at a time of our choosing.
  3. Adds #[diagnostic::on_unimplemented(...)] to FilterableValue for better diagnostics.
  4. Adds UI tests to ensure that .filter(..) and .find(...) impose FilterableValue.

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.

@Centril Centril requested a review from gefjon February 17, 2025 14:58
Copy link
Collaborator

@coolreader18 coolreader18 left a 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.

coolreader18
coolreader18 previously approved these changes Feb 18, 2025
@bfops
Copy link
Collaborator

bfops commented Feb 18, 2025

(Which ticket is this tied to?)

@Centril Centril force-pushed the centril/unique-index-args-must-be-filterable-value branch from ce8e61f to 370ec59 Compare February 18, 2025 18:18
@Centril Centril force-pushed the centril/unique-index-args-must-be-filterable-value branch from 370ec59 to 2456bcb Compare February 18, 2025 18:20
@cloutiertyler
Copy link
Contributor

I'm not sure if this is what was intended by the ticket, but if it is, this LGTM.

I think you need to be sure in order to approve the PR.

@coolreader18
Copy link
Collaborator

FTR: Mazdak said that @cloutiertyler said that this PR implements the intended behavior.

@coolreader18 coolreader18 dismissed their stale review February 18, 2025 18:28

maybe it's not the right thing

@coolreader18
Copy link
Collaborator

@gefjon this seems to be your ticket - could you review this PR to confirm if it's what you intended by #2272?

Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

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

Love the test!

@Centril Centril added this pull request to the merge queue Feb 18, 2025
Merged via the queue into master with commit 65ec786 Feb 18, 2025
14 checks passed
@Centril Centril deleted the centril/unique-index-args-must-be-filterable-value branch February 19, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent filtering with unique indexes of compound types in Rust modules
5 participants