PHPORM-273 Add schema helpers to create search and vector indexes #3230

GromNaN · 2024-12-18T12:34:37Z

The new helpers have advanced array-shape types for the index definition. The support for autocompletion of the array-shaped arguments in PHPStorm is not good for use-case 😞 (even if it was improved in 2022)

Checklist

Add tests and ensure they pass

jmikola · 2024-12-20T15:55:54Z

tests/SchemaTest.php

@@ -514,4 +558,16 @@ protected function getIndex(string $collection, string $name)

        return false;
    }
+
+    protected function getSearchIndex(string $collection, string $name)


Would it make more sense for this to return a nullable BSONDocument instance? IIRC, Collection::listSearchIndexes() inherits the default type map.

It's true that I simply copied the getIndex method. Should be named hasIndex and hasSearchIndex.

If this is supposed to be hasSearchIndex(), shouldn't it then just return a bool?

If there is actually a need to return the index definition, I'd suggest always returning a nullable BSONDocument and working with the return value accordingly in the calling context. But it looks like something should still be changed with this method.

tests/SchemaTest.php

jmikola · 2024-12-20T16:06:28Z

src/Schema/Blueprint.php

+ * @phpstan-type SearchIndexAnalyser array{name: string, charFilters?: list<array<string, mixed>>, tokenizer: array{type: string}, tokenFilters?: list<array<string, mixed>>}
+ * @phpstan-type SearchIndexStoredSource bool | array{includes: array<string>} | array{excludes: array<string>}
+ * @phpstan-type SearchIndexDefinition array{analyser?: string, analyzers?: SearchIndexAnalyser[], searchAnalyzer?: string, mappings: array{dynamic: true} | array{dynamic?: bool, fields: array<string, SearchIndexField>}, storedSource?: SearchIndexStoredSource}
+ * @phpstan-type VectorSearchIndexField array{type: 'vector', path: string, numDimensions: int, similarity: 'euclidean'|'cosine'|'dotProduct', quantization?: 'none'|'scalar'|'binary'}


https://www.mongodb.com/docs/atlas/atlas-vector-search/vector-search-type/ suggests that type may be either "vector" or "filter".

See also: https://www.mongodb.com/docs/manual/reference/command/createSearchIndexes/#vector-search-index-definition-syntax

src/Schema/Blueprint.php

jmikola · 2024-12-20T16:49:44Z

src/Schema/Blueprint.php

@@ -16,6 +16,14 @@
 use function is_string;
 use function key;

+/**
+ * @phpstan-type SearchIndexField array{type: 'boolean'|'date'|'dateFacet'|'objectId'|'stringFacet'|'uuid'} | array{type: 'autocomplete', analyzer?: string, maxGrams?: int, minGrams?: int, tokenization?: 'edgeGram'|'rightEdgeGram'|'nGram', foldDiacritics?: bool} | array{type: 'document'|'embeddedDocuments', dynamic?:bool, fields: array<string, array>} | array{type: 'geo', indexShapes?: bool} | array{type: 'number'|'numberFacet', representation?: 'int64'|'double', indexIntegers?: bool, indexDoubles?: bool} | array{type: 'token', normalizer?: 'lowercase'|'none'} | array{type: 'string', analyzer?: string, searchAnalyzer?: string, indexOptions?: 'docs'|'freqs'|'positions'|'offsets', store?: bool, ignoreAbove?: int, multi?: array<string, array>, norms?: 'include'|'omit'}
+ * @phpstan-type SearchIndexAnalyser array{name: string, charFilters?: list<array<string, mixed>>, tokenizer: array{type: string}, tokenFilters?: list<array<string, mixed>>}


Suggest using "SearchIndexAnalyzer". American English spelling should be preferred for consistency with the MongoDB documentation.

I'll note you had analyser?: string, analyzers?: SearchIndexAnalyser[] below, which is even internally inconsistent.

charFilters seems correct based on https://www.mongodb.com/docs/atlas/atlas-search/analyzers/character-filters/#std-label-char-filters-ref, although you could break that out into a separate definition to mandate type if you wish. Doing so would also give you a space to directly link to that documentation page. But considering how varied the structures are, mixed is also entirely reasonable.

Same feedback on tokenFilters (https://www.mongodb.com/docs/atlas/atlas-search/analyzers/token-filters/#std-label-token-filters-ref). Actually, it looks like both of those could utilize the same common definition (e.g. "charOrTokenFilter"), although that might send the wrong signal to users. I'm OK to keep this as-is.

src/Schema/Blueprint.php

jmikola · 2024-12-20T16:55:04Z

src/Schema/Blueprint.php

+ * @phpstan-type SearchIndexField array{type: 'boolean'|'date'|'dateFacet'|'objectId'|'stringFacet'|'uuid'} | array{type: 'autocomplete', analyzer?: string, maxGrams?: int, minGrams?: int, tokenization?: 'edgeGram'|'rightEdgeGram'|'nGram', foldDiacritics?: bool} | array{type: 'document'|'embeddedDocuments', dynamic?:bool, fields: array<string, array>} | array{type: 'geo', indexShapes?: bool} | array{type: 'number'|'numberFacet', representation?: 'int64'|'double', indexIntegers?: bool, indexDoubles?: bool} | array{type: 'token', normalizer?: 'lowercase'|'none'} | array{type: 'string', analyzer?: string, searchAnalyzer?: string, indexOptions?: 'docs'|'freqs'|'positions'|'offsets', store?: bool, ignoreAbove?: int, multi?: array<string, array>, norms?: 'include'|'omit'}
+ * @phpstan-type SearchIndexAnalyser array{name: string, charFilters?: list<array<string, mixed>>, tokenizer: array{type: string}, tokenFilters?: list<array<string, mixed>>}
+ * @phpstan-type SearchIndexStoredSource bool | array{includes: array<string>} | array{excludes: array<string>}
+ * @phpstan-type SearchIndexDefinition array{analyser?: string, analyzers?: SearchIndexAnalyser[], searchAnalyzer?: string, mappings: array{dynamic: true} | array{dynamic?: bool, fields: array<string, SearchIndexField>}, storedSource?: SearchIndexStoredSource}


Quoting the docs, mappings.fields is "required only if dynamic mapping is disabled." In that case, wouldn't it make sense to use dynamic?: false for that branch instead of a generic bool? I don't think there's any case where you'd specify dynamic: true and define fields.

It looks like synonyms is missing here. See: https://www.mongodb.com/docs/atlas/atlas-search/synonyms/#std-label-synonyms-ref

If it's allowed by the server, I think it's a bad idea to be more restrictive. In fact, dynamic: true and a list of fields is exactly what I do for Laravel Scout.

I'm not sure what file/line you were linking to above, as I have a lot of condensed files in that view. Was it the DEFAULT_DEFINITION using dynamic: true?

Even if you leave this as-is, I would inquire with the Atlas team about this as it's not clear from the documentation what dynamic: true with field mappings would actually imply. That it's accepted by the server might have been an oversight, or something they neglected to address in the docs.

src/Schema/Blueprint.php

jmikola · 2024-12-20T17:03:49Z

src/Schema/Blueprint.php

@@ -16,6 +16,14 @@
 use function is_string;
 use function key;

+/**
+ * @phpstan-type SearchIndexField array{type: 'boolean'|'date'|'dateFacet'|'objectId'|'stringFacet'|'uuid'} | array{type: 'autocomplete', analyzer?: string, maxGrams?: int, minGrams?: int, tokenization?: 'edgeGram'|'rightEdgeGram'|'nGram', foldDiacritics?: bool} | array{type: 'document'|'embeddedDocuments', dynamic?:bool, fields: array<string, array>} | array{type: 'geo', indexShapes?: bool} | array{type: 'number'|'numberFacet', representation?: 'int64'|'double', indexIntegers?: bool, indexDoubles?: bool} | array{type: 'token', normalizer?: 'lowercase'|'none'} | array{type: 'string', analyzer?: string, searchAnalyzer?: string, indexOptions?: 'docs'|'freqs'|'positions'|'offsets', store?: bool, ignoreAbove?: int, multi?: array<string, array>, norms?: 'include'|'omit'}


Corresponding docs for this type would be: https://www.mongodb.com/docs/atlas/atlas-search/define-field-mappings/#std-label-fts-field-mappings

I'm holding off reviewing this in case you're able to break it apart into multiple lines for readability.

tests/SchemaTest.php

src/Schema/Blueprint.php

jmikola · 2024-12-23T14:09:02Z

src/Schema/Blueprint.php

+ *     indexShapes?: bool,
+ * } | array{
+ *     type: 'number'|'numberFacet',
+ *     representation?: 'int64'|'double',


I'm not sure if there's a reason the docs used string instead of enum for this type: https://www.mongodb.com/docs/atlas/atlas-search/field-types/number-facet-type/#configure-fts-field-type-field-properties

Seems inconsistent with the autocomplete tokenization definition. Maybe something to alert the Atlas docs teams about. I'll defer to you if you'd rather relax this to string or restrict the values here.

I'll leave this thread open for the time being but I'm going to close my other comments about string and enum consistency.

src/Schema/Blueprint.php

jmikola · 2024-12-23T18:38:08Z

tests/SchemaTest.php

@@ -514,4 +558,16 @@ protected function getIndex(string $collection, string $name)

        return false;
    }
+
+    protected function getSearchIndex(string $collection, string $name)


If this is supposed to be hasSearchIndex(), shouldn't it then just return a bool?

If there is actually a need to return the index definition, I'd suggest always returning a nullable BSONDocument and working with the return value accordingly in the calling context. But it looks like something should still be changed with this method.

tests/TestCase.php

jmikola

Looks like all of my previous comments have been addressed.

GromNaN requested a review from jmikola December 18, 2024 12:34

GromNaN requested a review from a team as a code owner December 18, 2024 12:34

GromNaN force-pushed the PHPORM-273 branch from cc8bd9c to 48975a4 Compare December 18, 2024 12:38

jmikola reviewed Dec 20, 2024

View reviewed changes

tests/SchemaTest.php Outdated Show resolved Hide resolved

jmikola reviewed Dec 23, 2024

View reviewed changes

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

jmikola reviewed Dec 23, 2024

View reviewed changes

jmikola approved these changes Jan 2, 2025

View reviewed changes

GromNaN added 9 commits January 2, 2025 16:06

Add schema helpers to create search and vector indexes

6996cb9

multiline type definition

631141e

Skip tests that require Atlas Search

54bf40d

Add links to custom types

fc78500

Fix return type of getSearchIndex

073ecdf

Improve types

9912317

Add missing code in atlas search detection

80cea3b

Multiple mappings allowed per field name

e7c202d

Rebased on AtlasSearchTest

b38f88f

GromNaN force-pushed the PHPORM-273 branch from dfaf563 to b38f88f Compare January 2, 2025 17:00

Simplify search index definition type

7cf24fd

GromNaN merged commit 6cb3838 into mongodb:5.x Jan 2, 2025
27 checks passed

GromNaN deleted the PHPORM-273 branch January 2, 2025 17:13

GromNaN mentioned this pull request Jan 2, 2025

Add advanced type definition for Atlas Search Indexes #3234

Draft

GromNaN added the feature label Jan 2, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PHPORM-273 Add schema helpers to create search and vector indexes #3230

PHPORM-273 Add schema helpers to create search and vector indexes #3230

GromNaN commented Dec 18, 2024

jmikola Dec 20, 2024

GromNaN Dec 20, 2024

jmikola Dec 23, 2024

jmikola Dec 20, 2024

jmikola Dec 20, 2024 •

edited by GromNaN

Loading

jmikola Dec 20, 2024

GromNaN Dec 23, 2024 •

edited

Loading

jmikola Dec 23, 2024

jmikola Dec 20, 2024

jmikola Dec 23, 2024

jmikola Dec 30, 2024

jmikola Dec 23, 2024

jmikola left a comment

PHPORM-273 Add schema helpers to create search and vector indexes #3230

PHPORM-273 Add schema helpers to create search and vector indexes #3230

Conversation

GromNaN commented Dec 18, 2024

Checklist

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

jmikola Dec 20, 2024 • edited by GromNaN Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GromNaN Dec 23, 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

jmikola left a comment

Choose a reason for hiding this comment

jmikola Dec 20, 2024 •

edited by GromNaN

Loading

GromNaN Dec 23, 2024 •

edited

Loading