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

add some multirange operations support #4261

Merged
merged 12 commits into from
Oct 24, 2024

Conversation

guissalustiano
Copy link
Contributor

@guissalustiano guissalustiano commented Sep 13, 2024

Add multirange contains support under #4240

It is a small test to check the format before moving everything else

@guissalustiano guissalustiano requested a review from a team September 13, 2024 00:29
@guissalustiano guissalustiano changed the title add multirange contains support add some multirange operations support Sep 13, 2024
@guissalustiano guissalustiano marked this pull request as ready for review September 13, 2024 03:36
@guissalustiano guissalustiano marked this pull request as draft September 13, 2024 13:13
@guissalustiano
Copy link
Contributor Author

Hi @weiznich, sorry for keeping this PR open for so long.
I would like to use select to write some tests. It's simpler to write instead create a table, insert table and write some filters.
But I'm having some problems with how to use the range extension with literal value, the compiler doesn't know how to use the proper trait.

I want to write something like 063297d

@weiznich
Copy link
Member

weiznich commented Oct 3, 2024

No worries for taking some time on this.

I would like to use select to write some tests. It's simpler to write instead create a table, insert table and write some filters.
But I'm having some problems with how to use the range extension with literal value, the compiler doesn't know how to use the proper trait.

It's a good idea to skip the table setup as that should make the tests easier. As for the problem: I think something like this should work:

diesel::select((1..5).into_sql::<Range<Integer>>().contains(4)).load::<bool>(conn).unwrap();

The into_sql() call explicitly converts the rust side value into a SQL side expression (bind in this case), which then allows the DSL to work as expected.

@guissalustiano
Copy link
Contributor Author

guissalustiano commented Oct 8, 2024

Hi @weiznich, thanks for the patient. I think it adds a test to all the new behavior.
With this PR we can use the operations between Range and Range, or Multirange and Multirange.
I would like to also support operation between Multirange and Range (and vice versa), for that, I would need to change the T here to receive RangeOrMultirange, but I don't know how to do that. Could you help-me with that? (We can merge this PR and an open another one for that)

fn contains_range<T>(self, other: T) -> dsl::ContainsRange<Self, T>
where
Self::SqlType: SqlType,
T: AsExpression<Self::SqlType>,
{
Grouped(Contains::new(self, other.as_expression()))
}

@guissalustiano guissalustiano marked this pull request as ready for review October 8, 2024 03:28
Copy link
Member

@weiznich weiznich left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 🙏

@weiznich
Copy link
Member

I would like to also support operation between Multirange and Range (and vice versa), for that, I would need to change the T here to receive RangeOrMultirange, but I don't know how to do that. Could you help-me with that? (We can merge this PR and an open another one for that)

 fn contains_range<T>(self, other: T) -> dsl::ContainsRange<Self, T> 
where 
    Self::SqlType: SqlType, 
    T: AsExpression<Self::SqlType>, 

Technically this should be possible by introducing another generic parameter on the function signature above. The AsExpression<Self::SqlType> part essentially says: Give me something that can by turned in an SQL expression with the same SQL type (Range/Multirange) than the left hand side. We could now have a Rhs generic type there that just says, we expect some T that can be turned into the Rhs type, and expect that Rhs type to be either Range or MultiRange. Now the problem with that approach is that I'm not sure if that plays well with type inference. I would expect that the compiler than cannot infer the right type of Rhs on it's own, which would require that users always need to annotate these calls, which is not that great. I'm not sure on the last point, as I've not tried it yet, so it might be that rustc is clever enough to see that there is often just one type that fulfills this and does not require explict type annotations in that case.

@guissalustiano
Copy link
Contributor Author

I would like to also support operation between Multirange and Range (and vice versa), for that, I would need to change the T here to receive RangeOrMultirange, but I don't know how to do that. Could you help-me with that? (We can merge this PR and an open another one for that)

 fn contains_range<T>(self, other: T) -> dsl::ContainsRange<Self, T> 
where 
    Self::SqlType: SqlType, 
    T: AsExpression<Self::SqlType>, 

Technically this should be possible by introducing another generic parameter on the function signature above. The AsExpression<Self::SqlType> part essentially says: Give me something that can by turned in an SQL expression with the same SQL type (Range/Multirange) than the left hand side. We could now have a Rhs generic type there that just says, we expect some T that can be turned into the Rhs type, and expect that Rhs type to be either Range or MultiRange. Now the problem with that approach is that I'm not sure if that plays well with type inference. I would expect that the compiler than cannot infer the right type of Rhs on it's own, which would require that users always need to annotate these calls, which is not that great. I'm not sure on the last point, as I've not tried it yet, so it might be that rustc is clever enough to see that there is often just one type that fulfills this and does not require explict type annotations in that case.

Thanks so much for the explanation. Can we merge this PR?
I'll open another PR to explore this approach.

@weiznich
Copy link
Member

Can we merge this PR?

I will do that as soon as I fixed the CI. That turns out to be more problematic this time for whatever reason :(
Follow #4238 for updates.

@guissalustiano
Copy link
Contributor Author

Oh thanks!

@weiznich weiznich added this pull request to the merge queue Oct 24, 2024
Merged via the queue into diesel-rs:master with commit bdb8045 Oct 24, 2024
48 checks passed
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.

2 participants