implement MySQL Concat Fragment; do not set PIPES_AS_CONCAT by default

[thomasmost]

Vitess does not allow changing the parsing of MySQL queries themselves, and as such does not support the SQL modes "ANSI_QUOTES", "NO_BACKSLASH_ESCAPES", "PIPES_AS_CONCAT", and "REAL_AS_FLOAT"

https://github.com/vitessio/vitess/pull/10163/files#diff-9d2575386e98a47276e4d5c75e73cdefec7078a95ea4e159b50806a8c3099c1bR101

Vitess is a popular scalability layer on top of MySQL, used by default in PlanetScale, and we should support it by default (and either allow this mode to be added manually with config, or drop support for this mode)

[thomasmost]

Note that I'm happy to make this configurable if you'd like me to—but sometimes more configuration options aren't worth the hassle. Your call 🚀

[thomasmost]

note a similar PR in sqlx: https://github.com/launchbadge/sqlx/pull/2037/files

[weiznich]

Thanks for opening this PR. Diesel sets this option explicitly to maintain compatibility with other database systems. It might be possible to achieve such compatibility by generating different SQL for the mysql backend. It's likely not as simple as just disabling an option in the connection setup, as shown by the failing tests. I would expect such a PR to pass the test suite.

[thomasmost]

Thanks for opening this PR. Diesel sets this option explicitly to maintain compatibility with other database systems. It might be possible to achieve such compatibility by generating different SQL for the mysql backend. It's likely not as simple as just disabling an option in the connection setup, as shown by the failing tests. I would expect such a PR to pass the test suite.

yep, I see that. Will work on getting the tests passing

[thomasmost]

@weiznich I might need some assistance on this. I've identified this implementation of the concat query fragment here in expression/operators.rs:

impl<L, R, DB> QueryFragment<DB> for Concat<L, R>
where
    L: QueryFragment<DB>,
    R: QueryFragment<DB>,
    DB: Backend,
{
    fn walk_ast<'b>(
        &'b self,
        mut out: crate::query_builder::AstPass<'_, 'b, DB>,
    ) -> crate::result::QueryResult<()> {
        // Those brackets are required because mysql is broken
        // https://github.com/diesel-rs/diesel/issues/2133#issuecomment-517432317
        out.push_sql("(");
        self.left.walk_ast(out.reborrow())?;
        out.push_sql(" || ");
        self.right.walk_ast(out.reborrow())?;
        out.push_sql(")");
        Ok(())
    }
}

And I believe we need to do something like

    if mysql { 
        out.push_sql("CONCAT(");
        self.left.walk_ast(out.reborrow())?;
        out.push_sql(",");
        self.right.walk_ast(out.reborrow())?;
        out.push_sql(")");
        return Ok(())
    }

...but I'm not sure how to properly check the dialect on that first line. Can you offer guidance here?

[weiznich]

Such a conditional implementation needs to happen on type level. We have the SqlDialect trait as super trait of Backend for this.
See here:

diesel/diesel/src/backend.rs

Line 149 in 17c1bb4

pub trait SqlDialect: self::private::TrustedBackend {

#3257 introduces a such a type level flag for another usecase, so it can be use example of how to introduce such a type-level flag. This allows to just provide different wild card implementations of QueryFragment for a Concat which allows to have completely different implementations for different backends.

[thomasmost]

@weiznich I see that PR is Open—do I need to wait for it to be merged?

[thomasmost]

like it would be something like this?

    #[diesel_derives::__diesel_public_if(
        feature = "mysql"
    )]

?

[thomasmost]

Well, that didn't work. Can you tell me what I'm doing wrong here?

[weiznich]

This is not unexpected as this is just not the way I've suggested above 😉. Your implementation will fail as soon as some user enables more than on backend for diesel, which is to be clear a valid usecase. Follow the example given above and it should work.

To answer the other questions quickly:

I see that PR is Open—do I need to wait for it to be merged?

No you don't need for that to be merged. That's just an example case of how to introduce such a type level flag as required here. You need to do the "same" thing for SqlDialect + a similar adjustment to the relevant QueryFragment impl (with the actual function implementation being totally different). This PR just shows how that is done in another unrelated case. They can be merged without depending on each other (beside potential merge conflicts).
As a probably important side note: The customized QueryFragment impl for mysql should be part of src/mysql/query_builder + the custom marker type should be internal to src/mysql (and likely defined in src/mysql/backend.rs.

like it would be something like this?

   #[diesel_derives::__diesel_public_if(
       feature = "mysql"
   )]

No you don't need that at all. This attribute macro toggles whether something is public or not depending on the passed cfg expression. This is not meaningful for the mysql feature, but relevant for the i-implement-a-third-party-backend-and-opt-into-breaking-changes (as custom backends need to have access to those otherwise unstable internals). The added associated type for SqlDialect should just look like any other type defined there.

[thomasmost]

Got it, let me see if I can make those adjustments...

[thomasmost]

Okay, @weiznich — am I at least heading in the right direction now?

[thomasmost]

am I at least heading in the right direction now?

Okay, I think that's a yes 🙂 @weiznich can you help me get this over the finish line? I am having trouble running trybuild locally and I'm having trouble parsing what the failure is...

[weiznich]

am I at least heading in the right direction now?

Okay, I think that's a yes 🙂 @weiznich can you help me get this over the finish line?

Yes that's the right approach, you've just asked that in the middle of my night.
I will leave some inline comments about minor things that need to be changed. (Mostly minor things + documentation)

I am having trouble running trybuild locally and I'm having trouble parsing what the failure is...

This is likely caused by some minor change to an error message due to the changed internal implementations. You can fix that by just accepting and committing the changes via TRYBUILD=overwrite cargo test in diesel_compile_tests. The relevant part of the error message is that one:

note: If the actual output is the correct output you can bless it by rerunning
your test with the environment variable TRYBUILD=overwrite

See here

[weiznich]

Thanks for updating the implementation. The actual code looks fine, but the documentation needs to be improved.

In addition to the two inline comments this also needs an changelog entry that mentions the removal of setting PIPES_AS_CONCAT as concat operator on connection setup. This is a potentially breaking change for people that use || as concat in raw sql queries. We need to communicate this so that users can either adapt their code or manually enable that option on their own.

[weiznich]

I would prefer a name here that indicates that any backend using this impl uses || as concat operator. Additionally the documentation should also mention this explicitly.

[thomasmost]

✅