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

.eq_any(ARRAY(subselect)) and .eq_any(ARRAY[..., ...]) support #4353

Merged
merged 26 commits into from
Nov 28, 2024

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented Nov 20, 2024

As underlined here, there are cases where = ANY(array) does not behave the same as = ANY(subquery), so it may be useful to explicitly convert a subquery into an array.

This PR allows doing this.

It adds support for:

  • Constructing an array from a subselect with array(subselect)
  • Using outputs of array(...) in .eq_any(...) and .ne_all(...) expressions

It's technically a breaking change, but only to the extent to which people implemented the AsExpressionList trait manually, or wrote new query fragments that use this as bound, which seems unlikely enough that this can probably be released. (That's only a marker trait for usage of array(...) in its current somewhat-limited version.)

@Ten0 Ten0 requested a review from a team November 20, 2024 13:48
Comment on lines 82 to 93
/// Implement as a no-op for things that were already arrays (that is, don't wrap in ARRAY[]).
impl<ST, T> IntoArrayExpression<ST> for T
where
T: AsExpressionList<ST>,
T: AsExpression<sql_types::Array<ST>>,
ST: SqlType + TypedExpressionType + 'static,
{
ArrayLiteral {
elements: elements.as_expression_list(),
_marker: PhantomData,
type ArrayExpression = <T as AsExpression<sql_types::Array<ST>>>::Expression;

fn into_array_expression(self) -> Self::ArrayExpression {
<T as AsExpression<sql_types::Array<ST>>>::as_expression(self)
}
}
Copy link
Member Author

@Ten0 Ten0 Nov 20, 2024

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 we should have this impl. Maybe not, if we want to map as close as SQL as possible and forbid writing array(array_expr) for something that's actually going to get serialized as just array_expr.
It seems easier to add later on than to remove.
On the other hand it seems unlikely to be hurtful.

This removes the fake Expression implementation for `Many`, instead relying on an `InExpression` trait (which replaces `MaybeEmpty`) that represents that that query fragment is valid for use inside `IN` or `= ANY`
@Ten0 Ten0 changed the title Add support for subselects to the array(...) function .eq_any(ARRAY(subselect)) support Nov 20, 2024
@Ten0 Ten0 changed the title .eq_any(ARRAY(subselect)) support .eq_any(ARRAY(subselect)) and .eq_any(ARRAY[..., ...]) support Nov 20, 2024
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 for me 👍

///
/// It has several quirks:
/// - `All<Expr: Expression<SqlType = Array<ST>>>` pretends to be an expression of type `ST`,
/// but it's in fact some custom that can really be only used in combination with the `=`
Copy link
Member

Choose a reason for hiding this comment

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

Technically it can also be used with different operators, e.g. like, which is something that we still not support without the deprecated functions

@weiznich weiznich added this pull request to the merge queue Nov 28, 2024
Merged via the queue into diesel-rs:master with commit 8939fce Nov 28, 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