Skip to content

PHPORM-274 List search indexes in Schema::getIndexes() introspection method #3233

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 2, 2025

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Dec 31, 2024

Fix PHPORM-274

Search indexes are a distinct type of index that are exposed using a specific method. Listing the search indexes in Schema::getIndexes() so they are listed by the command php artisan db:table.

@GromNaN GromNaN requested a review from a team as a code owner December 31, 2024 13:28
@GromNaN GromNaN requested a review from jmikola December 31, 2024 13:28
use function assert;
use function usleep;

class AtlasSearchTest extends TestCase
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to test all the search index features in this class. It is also in #3232

Copy link
Member

@jmikola jmikola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions but LGTM. Feel free to add a test for dynamic field mappings if you wish.

'name' => $index['name'],
'columns' => match ($index['type']) {
'search' => array_keys($index['latestDefinition']['mappings']['fields'] ?? []),
'vectorSearch' => array_column($index['latestDefinition']['fields'], 'path'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per Atlas Vector Search Index Fields, I expect this might return field paths instead of field names. Is that going to cause problems with whatever consumes this?

I assume it may be consistent with how the basic listIndexes() query above runs (using array_keys($index->getKey()) but I'm not sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as it is a string, it can be displayed.

$indexList[] = [
'name' => $index['name'],
'columns' => match ($index['type']) {
'search' => array_keys($index['latestDefinition']['mappings']['fields'] ?? []),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need better reporting for dynamic indexes, or is there no good way to express that given the expected format of $indexList elements?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All I can do is returning "dynamic" as a column name. This is purely informative.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning dynamic as a column name seems undesirable, as then it might be indistinguishable from a real index with a dynamic field mapped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alternative is replace the type with search, dynamic

$indexList = [];

$indexes = $collection->listIndexes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is outside the diff but I think it's worth mentioning since I see you use an === to check for ['_id' => 1]. If there is ever an issue with that, you may need to relax the comparison on the numeric value. IIRC, mongos used to sometimes return command results as 1.0 instead of 1 and I think there may be similar inconsistencies for index definitions.

That said, it's probably fine to just wait and see if this is ever reported before going out of your way to address it now. It's quite possible that it's a non-issue on recent server versions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The triple === is enforced by phpcs. We will fix it if a new server version produces an incompatible value, but I doubt it.

@GromNaN
Copy link
Member Author

GromNaN commented Jan 2, 2025

Example of output:

❯ php artisan db:table comments

  comments ................................................................................  
  Columns ............................................................................... 7  
  Size ........................................................................... 52.00 KB  

  Column ............................................................................. Type  
  id objectId, objectId .......................................................... objectId  
  author_id string, nullable ....................................................... string  
  content string, nullable ......................................................... string  
  created_at date, nullable .......................................................... date  
  post_id string, nullable ......................................................... string  
  published_at date, nullable ........................................................ date  
  updated_at date, nullable .......................................................... date  

  Index ...................................................................................  
  _id_ _id ........................................................................ primary  
  vector_index my_vector ..................................................... vectorSearch  
  default dynamic .................................................................. search  
  search dynamic, body ............................................................. search  


'name' => $index['name'],
'columns' => match ($index['type']) {
'search' => array_merge(
$index['latestDefinition']['mappings']['dynamic'] ? ['dynamic'] : [],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • latestDefinition.mappings.dynamic is always defined.
  • dynamic word is added to the list for columns names it there is also explicit mapping.

@GromNaN GromNaN merged commit 7890518 into mongodb:5.x Jan 2, 2025
27 checks passed
@GromNaN GromNaN deleted the PHPORM-274 branch January 2, 2025 13:41
@GromNaN GromNaN added the feature label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants