Mysql ddl compiler fall back to default index args

[TiansuYu]

Description

Fixes: #13134

Checklist

This pull request is:

• A documentation / typographical / small typing error fix
  • Good to go, no issue or tests are needed
• A short code fix
  • please include the issue number, and create an issue if none exists, which
    must include a complete example of the issue. one line code fixes without an
    issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.
• A new feature implementation
  • please include the issue number, and create an issue if none exists, which must
    include a complete example of how the feature would look.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests.

Have a nice day!

[zzzeek]

thanks, this looks really good, ill continue review

[sqla-tester]

OK, this is sqla-tester setting up my work on behalf of zzzeek to try to get revision 1bc953a of this pull request into gerrit so we can run tests and reviews and stuff

[sqla-tester]

New Gerrit review created for change 1bc953a: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[zzzeek]

the oracle failures are unrelated, it has some nox -s pep8 fails (we require black to be run on all files). installing pre-commit would fix. let me know if you want me to finish this PR up including the backport

[sqla-tester]

Gord Thompson (gordthompson) wrote:

This appears to be the only thing that black would want to fix:

Index: test/dialect/mysql/test_compiler.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/test/dialect/mysql/test_compiler.py b/test/dialect/mysql/test_compiler.py
--- a/test/dialect/mysql/test_compiler.py	(revision 405f7faad9c422578272e1c0ee04e1c828978325)
+++ b/test/dialect/mysql/test_compiler.py	(date 1771608649557)
@@ -269,12 +269,8 @@
     def test_create_index_with_length(self, dialect_name):
         m = MetaData()
         tbl = Table("testtbl", m, Column("data", String(255)))
-        idx1 = Index(
-            "test_idx1", tbl.c.data, **{f"{dialect_name}_length": 10}
-        )
-        idx2 = Index(
-            "test_idx2", tbl.c.data, **{f"{dialect_name}_length": 5}
-        )
+        idx1 = Index("test_idx1", tbl.c.data, **{f"{dialect_name}_length": 10})
+        idx2 = Index("test_idx2", tbl.c.data, **{f"{dialect_name}_length": 5})
 
         self.assert_compile(
             schema.CreateIndex(idx1),

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Gord Thompson (gordthompson) wrote:

If the changelog is okay then I can do the backport, too. (I've never felt very confident at writing changelog entries.)

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Gord Thompson (gordthompson) wrote:

Done

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[TiansuYu]

I have fixed the lint. I am unaware of what needs to be back-ported. If someone can point that out to me, I can add that too. Otherwise, feel free to go ahead.

[sqla-tester]

Michael Bayer (zzzeek) wrote:

code review left on gerrit

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Michael Bayer (zzzeek) wrote:

i wanted to make sure we had a positive test for the actual original problem.

this uses a new fixture to make the dialect reg thing easier

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Federico Caselli (CaselIT) wrote:

Looks ok to me

For 2.0 this is kinda a breaking change, since using mysql_lenght with mariadb or other dialect no longer works. Maybe we could better spell that out in the change note?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Michael Bayer (zzzeek) wrote:

this is a good point and i think instead we should have a fallback for now

we dont need more mysql regressions

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Michael Bayer (zzzeek) wrote:

can you review again?

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[sqla-tester]

Federico Caselli (CaselIT) wrote:

that's what I had in mind too

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601

[TiansuYu]

This might be a naive suggestion:

To make this backward compatible, e.g.

parser = index.get_dialect_option(self.dialect, "with_parser")

could be changed to

parser = index.get_dialect_option(self.dialect, "with_parser") or index.get_dialect_option("mysql", "with_parser")

so the hard-coded parts for mariadb could still be functional. Similarly for other arguments. Would this be a valid option?

[CaselIT]

the code in the PR is outdated, it was already updated on gerrit.

there is a new parameter in get_dialect_option: using = constraint.get_dialect_option(self.dialect, "using", deprecated_fallback="mysql").

No further change is expect from you, thanks for the PR!

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6601 has been merged. Congratulations! :)

[sqla-tester]

Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/6605 has been merged. Congratulations! :)