Ignore index prefix lengths for spatial indexes

[ksaveras]

Q	A
Type	improvement
BC Break	no
Fixed issues	#3561

Summary

MySQL spatial indexes do not support index lengths and this check should be skipped.
If there are MSSQL or PGSQL experts - please comment.

This PR skips index lengths check if both indexes have a spatial flag.

I.e. when an index is created, MySQL 5.7 creates with 32 bytes prefix length. AWS Aurora (MySQL 5.7 compatible) - 24 bytes prefix length.

[cesurapp]

This development works for the spatial index.

[Apostropher]

Is there an ETA for when this will be merged? I'm currently waiting for this fix.

[greg0ire]

Not a PG expert, but it looks like this wouldn't be handled by flags in PG since the syntax, which requires the PostGis extension is :

CREATE INDEX nyc_census_blocks_geom_idx
  ON nyc_census_blocks
  USING GIST (geom);

flags are apparently supposed to appear between CREATE and INDEX.

PostGIS does not seem to be supported though (and will never be?)

[greg0ire]

Also, the linked issue shows that this piece of code is responsible for the length appearing:

dbal/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php

Lines 69 to 71 in 22800bd

	$v['flags'] = ['SPATIAL'];
	}
	$v['length'] = $v['sub_part'] ?? null;

Why not change it so that the length is not set at all in case the index is spatial?

[morozov]

Since this is a data provider for a spatial index test, why do we care about testing the length comparison? If the logic of comparison of the flags and lengths needs to be tested, it's better to do it in a separate test that would not get too much into the details of why the lengths or flags mismatch.

[ksaveras]

We are testing if the method isFullfilledBy works as expected in different cases. When spatial index created or migration is generated (or checked) on different platforms: MySQL, AWS Aurora MySQL.

[morozov]

This is fine but some of these different cases are better to be implemented as a separate test method. Otherwise, if someone breaks the length comparison logic and testFulfilledSpatialIndex() fails, it will be misleading feedback from the test suite.

[SenseException]

Using a text key on data providers would also give more information to a failing test with a data row, but separate test cases provide a more contextual structure for the test class.

[ksaveras]

Also, the linked issue shows that this piece of code is responsible for the length appearing:

dbal/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php

Lines 69 to 71 in 22800bd

	$v['flags'] = ['SPATIAL'];
	}
	$v['length'] = $v['sub_part'] ?? null;

Why not change it so that the length is not set at all in case the index is spatial?

I like this idea.

[greg0ire]

@ksaveras 2.10.x only accepts security fixes now, retargeting to 2.11.x .

[greg0ire]

git fetch
git rebase origin/2.11.x
composer install
vendor/bin/phpcbf

should help with the cs issues.

[ksaveras]

Thanks @greg0ire
Do you want to add this globally for all contributors?
https://docs.github.com/en/free-pro-team@latest/github/building-a-strong-community/setting-guidelines-for-repository-contributors

[greg0ire]

Do you want to add this globally for all contributors?

We already have a contributing guide, but no link to it, which sounds a bit weird. Good point, I will make a PR later to address that.

UPD: See #4299

[greg0ire]

Travis got confused with the 2 PRs, let's close and reopen to trigger a new build

[morozov]

Sorry, I misunderstood the boolean logic being tested when requested the changes in #3970 (comment). Makes sense.

[morozov]

[…] when an index is created, MySQL 5.7 creates with 32 bytes prefix length. AWS Aurora (MySQL 5.7 compatible) - 24 bytes prefix length.

While the patch addresses this issue, I can hardly see it being a bug. DBAL does not claim the support for portability between MySQL and Aurora, in which case it would be a bug.

I don't know enough about how index length is applied to spatial indexes, so it is possible that it's a breaking change while the primary focus of 2.11.1 is to fix the existing BC breaks before the 3.0 release.

UPD: as pointed out in the documentation mentioned in #3561.

Column prefix lengths are prohibited. The full width of each column is indexed.

So it is sort of a bug.

[morozov]

@ksaveras what does MySQL report as the spatial index sub_part if the column length is prohibited? Should we, instead of changing the comparison logic, check the introspection logic?

dbal/lib/Doctrine/DBAL/Schema/MySqlSchemaManager.php

Lines 66 to 71 in 22800bd

	if (strpos($v['index_type'], 'FULLTEXT') !== false) {
	$v['flags'] = ['FULLTEXT'];
	} elseif (strpos($v['index_type'], 'SPATIAL') !== false) {
	$v['flags'] = ['SPATIAL'];
	}
	$v['length'] = $v['sub_part'] ?? null;

E.g. not use sub_part to populate the length of a spatial index.

[ottaviano]

Hi, we have the same issue. How can we help getting merged this PR?

[morozov]

@ottaviano this patch requires integration testing. This may shed some light on how #3970 (comment) should be addressed.

[morozov]

Superseded by #4525.