Added support functional indexes for MySQL and Postgres #6414

prohalexey · 2024-05-29T14:52:13Z

Q	A
Type	feature

Summary

This feature allows the DBAL to create and work with functional indexes.

Documentation:
PostgreSQL (version 7 and higher) - https://www.postgresql.org/docs/7.3/indexes-functional.html
MySQL (version 8.0.13 and higher) - https://dev.mysql.com/doc/refman/8.0/en/create-index.html#create-index-functional-key-parts

prohalexey · 2024-06-03T09:37:29Z

@derrabus @greg0ire could you run checks?

derrabus

Thank you. I have to do a deeper review.

As this is not a bugfix, so please target the next feature release (4.1.x currently). I've retargeted your PR, but you'll need to rebase and resolve conflicts. The reason for that is that we've already introduced new MySQL platform classes in 4.1.x. Also, we don't want merge commits in PRs. A rebase should get rid of those.

src/Driver/AbstractMySQLDriver.php

src/Platforms/AbstractPlatform.php

src/Schema/PostgreSQLSchemaManager.php

derrabus · 2024-06-06T11:25:36Z

The test failures appear to be related to your changes.

prohalexey · 2024-06-06T12:17:48Z

@derrabus Please pay attention to MySQL platform inheritance. Did I do it as you said?

prohalexey · 2024-06-07T07:31:35Z

@derrabus Do we have to restart test?

derrabus · 2024-06-07T07:49:54Z

Don't mind the failing CodeCov upload.

prohalexey · 2024-06-12T07:48:47Z

@derrabus Is where any news ? Could i add any additional information ?

derrabus · 2024-06-12T17:32:56Z

I need to find the time for a review. Soon, I hope.

derrabus

Thank you for this promising contribution. When we add an abstraction for database feature, we should try to implement that abstraction for all platforms that support that feature. Otherwise we might have chosen the wrong abstraction without knowing it.

You've implemented the abstraction for MySQL and Postgres only, but if I'm not mistaken, all other platforms (except for MariaDB) should support functional indexes in some shape or form. Could you look into those as well?

tests/Functional/Platform/FunctionalIndexTest.php

derrabus · 2024-06-19T12:40:41Z

tests/Functional/Platform/FunctionalIndexTest.php

+        $table->addColumn('column2', Types::INTEGER, ['notnull' => false]);
+        $table->addIndex(['column1', 'column2', '(column2 IS NOT NULL)'], 'func_idx');
+
+        $this->expectException(InvalidArgumentException::class);


We should have an opinion on the exception message as well.

The text of exception is created dynamically so we should not check it? Maybe we could extend InvalidArgumentException and name it as FunctionalIndexNotSupportedException?

Especially when the message is created dynamically, we need an assertion for it. How would I make sure otherwise that the correct exception message is rendered? In this particular case, we should have all the information we need to compute the exception message we expect.

Ok. Added check

tests/Functional/Platform/FunctionalIndexTest.php

src/Platforms/AbstractPlatform.php

src/Platforms/MySQL8013Platform.php

derrabus · 2024-06-19T13:00:09Z

src/Platforms/SQLitePlatform.php

@@ -561,6 +562,16 @@ public function getCreateIndexSQL(Index $index, string $table): string
            ));
        }

+        if ($index->isFunctional() && ! $this->supportsFunctionalIndex()) {


We're inside a platform class for SQLite. We should know whether SQLite supports functional indexes.

derrabus · 2024-06-19T13:02:23Z

src/Schema/Index.php

@@ -101,10 +109,14 @@ public function getQuotedColumns(AbstractPlatform $platform): array
        foreach ($this->_columns as $column) {
            $length = array_shift($subParts);

-            $quotedColumn = $column->getQuotedName($platform);
+            if ($this->isFunctional()) {
+                $quotedColumn = $column->getName();


A functional column does not have a name. We should find a better representation for such columns.

And which representation is better :) Even in databases, functional indexes are stored in a hackish way.

derrabus · 2024-06-19T13:07:18Z

src/Platforms/MySQL8013Platform.php

+{
+    public function getColumnNameForIndexFetch(): string
+    {
+        return "COALESCE(COLUMN_NAME, CONCAT('(', REPLACE(EXPRESSION, '\\\''', ''''), ')'))";


This looks unnecessarily complicated. Can't we just select the COLUMN_NAME and EXPRESSION columns as separate columns?

I doesn't make sense, because expression could be a part of composite index for example: (column, column, expression, column). So in term of code it is a column

I understand that, but that does not really answer my question.

src/Driver/AbstractMySQLDriver.php

derrabus · 2024-06-19T14:00:48Z

src/Driver/AbstractMySQLDriver.php

+            Deprecation::trigger(
+                'doctrine/dbal',
+                'https://github.com/doctrine/dbal/pull/6343',
+                'Support for MySQL <= 8.0.13 is deprecated and will be removed in DBAL 5',
+            );


If you move the deprection trigger above the if statement, you can remove the other deprecation further below. Please also add a note to our upgrade guide about the new deprecation.

Please look it closer, I think I'm made it right. There is a mariadb branch, mysql 8.0 and higher and default branch for lower(5.7 is still supported, despite of deprecation).

After your change, we have a deprecation message for MySQL < 8.0.13 and another one for MySQL < 8.0. The second one is redundant.

prohalexey · 2024-06-25T13:37:21Z

Thanks for your feedback, I'll take care of your comments. Not sure about another databases, but I'll look into it.

derrabus · 2024-07-08T19:56:00Z

tests/Functional/Platform/FunctionalIndexTest.php

+        if (! $platform->supportsFunctionalIndex()) {
+            self::markTestSkipped('Platform does not support functional indexes.');
+        }


Again, in our tests we should know which platform supports functional indexes. We should not ask the platform.

derrabus · 2024-07-08T19:57:53Z

tests/Functional/Platform/FunctionalIndexTest.php

+        if ($platform instanceof PostgreSQLPlatform) {
+            self::assertEquals(['column1', 'column2', '(column2 IS NOT NULL)'], $index->getColumns());
+        } elseif ($platform instanceof MySQLPlatform) {
+            self::assertEquals(['column1', 'column2', '(`column2` is not null)'], $index->getColumns());
+        }


And what if the platform is neither PostgreSQLPlatform nor MySQLPlatform?

derrabus · 2024-07-08T20:00:42Z

tests/Schema/Platforms/MySQL8013SchemaTest.php

+        foreach ($table->getIndexes() as $index) {
+            $this->expectException(Exception::class);
+
+            $platform->getCreateIndexSQL(
+                $index,
+                $table->getQuotedName($platform),
+            );
+        }


This loop does not make sense. The test will exit at the first exception. At this point, we should know…

… how many indexes the table has.

… which of the raises the exception.

derrabus · 2024-07-08T20:03:21Z

src/Platforms/AbstractPlatform.php

+    /**
+     * A flag that indicates whether the platform supports functional indexes.
+     */
+    public function supportsFunctionalIndex(): bool
+    {
+        return true;
+    }


Repeating myself: I don't think we need this supports() function. We've removed a lot of those and I'm not eager to add any new ones.

Why we have supportsPartialIndexes but can't have supportsFunctionalIndex ?
What's the difference ?

The difference is that the method in question was introduced in 2014 and we have 2024 now. Quoting myself:

We've removed a lot of those and I'm not eager to add any new ones.

I can't figure out what the alternative is? If you told me this approach is wrong, what is the right approach?
There are databases that do not supports functional indexes, so we must have a way to check and know that.

I can change the code, but I can't read your thoughts on it.

I can't figure out what the alternative is?

Create a new method in the platform classes that renders DDL for a functional index. Let it throw by default. Override that method inside the platform classes that support functional indexes.

There are databases that do not supports functional indexes, so we must have a way to check and know that.

No, we don't.

derrabus · 2024-07-08T20:08:31Z

Functional tests that are missing: Schema comparison and migration.

prohalexey force-pushed the functionalIndexes branch 3 times, most recently from 3e07ddf to 53b632c Compare May 31, 2024 09:01

prohalexey force-pushed the functionalIndexes branch from 53b632c to fb58217 Compare June 6, 2024 07:26

derrabus changed the base branch from 4.0.x to 4.1.x June 6, 2024 08:01

derrabus requested changes Jun 6, 2024

View reviewed changes

src/Driver/AbstractMySQLDriver.php Outdated Show resolved Hide resolved

src/Driver/AbstractMySQLDriver.php Show resolved Hide resolved

src/Platforms/AbstractPlatform.php Outdated Show resolved Hide resolved

src/Schema/PostgreSQLSchemaManager.php Outdated Show resolved Hide resolved

derrabus added New Feature Schema Introspection Schema Definition Indexes labels Jun 6, 2024

prohalexey force-pushed the functionalIndexes branch 2 times, most recently from 586dfcb to dfef783 Compare June 6, 2024 09:13

prohalexey requested a review from derrabus June 6, 2024 09:15

Added support of functional indexes for MySQL and Postgres

6484bf9

prohalexey force-pushed the functionalIndexes branch from dfef783 to 6484bf9 Compare June 6, 2024 12:16

derrabus added this to the 4.1.0 milestone Jun 12, 2024

derrabus requested changes Jun 19, 2024

View reviewed changes

derrabus reviewed Jun 19, 2024

View reviewed changes

src/Driver/AbstractMySQLDriver.php Outdated Show resolved Hide resolved

derrabus reviewed Jun 19, 2024

View reviewed changes

Code review changes

3fba759

derrabus removed this from the 4.1.0 milestone Jul 8, 2024

derrabus requested changes Jul 8, 2024

View reviewed changes

derrabus added the Missing Documentation label Jul 8, 2024

derrabus added the Missing Tests label Jul 8, 2024

derrabus changed the base branch from 4.1.x to 4.2.x August 15, 2024 09:17

derrabus changed the base branch from 4.2.x to 4.3.x October 10, 2024 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support functional indexes for MySQL and Postgres #6414

Added support functional indexes for MySQL and Postgres #6414

prohalexey commented May 29, 2024

prohalexey commented Jun 3, 2024

derrabus left a comment •

edited

Loading

derrabus commented Jun 6, 2024

prohalexey commented Jun 6, 2024

prohalexey commented Jun 7, 2024

derrabus commented Jun 7, 2024

prohalexey commented Jun 12, 2024

derrabus commented Jun 12, 2024

derrabus left a comment

derrabus Jun 19, 2024

prohalexey Jun 27, 2024

derrabus Jul 8, 2024

prohalexey Oct 14, 2024

derrabus Jun 19, 2024

derrabus Jun 19, 2024

prohalexey Oct 14, 2024

derrabus Jun 19, 2024

prohalexey Jun 27, 2024

derrabus Jul 8, 2024

derrabus Jun 19, 2024

prohalexey Jun 27, 2024

derrabus Jul 8, 2024

prohalexey commented Jun 25, 2024

derrabus Jul 8, 2024 •

edited

Loading

derrabus Jul 8, 2024

derrabus Jul 8, 2024

derrabus Jul 8, 2024

prohalexey Oct 14, 2024

derrabus Oct 14, 2024

prohalexey Oct 14, 2024

prohalexey Oct 14, 2024

derrabus Oct 14, 2024

derrabus commented Jul 8, 2024

Added support functional indexes for MySQL and Postgres #6414

Are you sure you want to change the base?

Added support functional indexes for MySQL and Postgres #6414

Conversation

prohalexey commented May 29, 2024

Summary

prohalexey commented Jun 3, 2024

derrabus left a comment • edited Loading

Choose a reason for hiding this comment

derrabus commented Jun 6, 2024

prohalexey commented Jun 6, 2024

prohalexey commented Jun 7, 2024

derrabus commented Jun 7, 2024

prohalexey commented Jun 12, 2024

derrabus commented Jun 12, 2024

derrabus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prohalexey commented Jun 25, 2024

derrabus Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Jul 8, 2024

derrabus left a comment •

edited

Loading

derrabus Jul 8, 2024 •

edited

Loading