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 Cast operator #4024

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Add Cast operator #4024

wants to merge 1 commit into from

Conversation

Ten0
Copy link
Member

@Ten0 Ten0 commented May 14, 2024

Implemented this, then realized I in fact didn't need it, figured that a draft PR would be a good place to put it nonetheless so that we don't lose the done work if it becomes needed in the future.

I'm open to not merging this and even closing the PR as "not needed".
All of this could also be implemented outside of Diesel.

@Ten0 Ten0 force-pushed the cast_operator branch 4 times, most recently from 074fb91 to 5cf4c23 Compare May 14, 2024 18:27
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.

I'm generally in favor of adding this to diesel itself. The implementation also looks fine beside the noted minor changes.

Additionally I would appreciate that we add the new expression to the auto_type test module in diesel_derives just to ensure that it is compatible.

@@ -37,6 +37,7 @@ pub(crate) mod nullable;
#[macro_use]
pub(crate) mod operators;
mod case_when;
pub mod cast;
Copy link
Member

Choose a reason for hiding this comment

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

I would like to keep the module private and just reexport the necessary items from the expression module.

/// Marker trait: this SQL type (`Self`) can be casted to the target SQL type
/// (`ST`) using `CAST(expr AS target_sql_type)`
pub trait CastsTo<ST> {}
impl<ST1, ST2> CastsTo<sql_types::Nullable<ST2>> for sql_types::Nullable<ST1> where ST1: CastsTo<ST2>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
impl<ST1, ST2> CastsTo<sql_types::Nullable<ST2>> for sql_types::Nullable<ST1> where ST1: CastsTo<ST2>
impl<ST1, ST2> CastsTo<sql_types::Nullable<ST2>> for sql_types::Nullable<ST1> where ST1: CastsTo<ST2>

/// `Self`
fn sql_type_name() -> &'static str;
}
impl<ST, DB> KnownCastSqlTypeName<DB> for sql_types::Nullable<ST>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
impl<ST, DB> KnownCastSqlTypeName<DB> for sql_types::Nullable<ST>
impl<ST, DB> KnownCastSqlTypeName<DB> for sql_types::Nullable<ST>

#[diagnostic::on_unimplemented(
note = "In order to use `CAST`, it is necessary that Diesel knows how to express the name \
of this type in the given backend.",
note = "This can be PRed into Diesel if the type is a standard SQL type."
Copy link
Member

Choose a reason for hiding this comment

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

We likely want to change this message to something more active like: If you run into this error message and believe that this cast should be supported open a PR that adds that trait implementation there (with link to the source or file path)

{
type SqlType = ST;
}
impl<E, ST, DB> query_builder::QueryFragment<DB> for Cast<E, ST>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
impl<E, ST, DB> query_builder::QueryFragment<DB> for Cast<E, ST>
impl<E, ST, DB> query_builder::QueryFragment<DB> for Cast<E, ST>

E: FieldAliasMapper<S>,
{
type Out = Cast<<E as FieldAliasMapper<S>>::Out, ST>;
fn map(self, alias: &query_source::Alias<S>) -> Self::Out {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn map(self, alias: &query_source::Alias<S>) -> Self::Out {
fn map(self, alias: &query_source::Alias<S>) -> Self::Out {

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