Skip to content
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

Feature Request: Compound Full-Text Indexes #875

Closed
wants to merge 1 commit into from

Conversation

jpedryc
Copy link

@jpedryc jpedryc commented Nov 1, 2024

Note

Related to previous requests in #724 and #833.


Overview

Assuming we're having a coumpound full-text index like:

ALTER TABLE `t` ADD FULLTEXT test_index(firstname, lastname, internal_clientnumber);

Old behaviour

    #[SearchUsingFullText([
        'firstname',
        'lastname',
        'internal_clientnumber'
    ])]
    public function toSearchableArray(): array
    {
        return [
            'firstname'          => $this->firstname,
            'lastname'           => $this->lastname,
            'internal_clientnumber' => $this->internal_clientnumber,
            'generated_fullname' => $this->firstname . $this->lastname,
            'email'              => $this->email,
        ];
    }

SQLSTATE[HY000]: General error: 1191 Can't find FULLTEXT index matching the column list ...

New behaviour

    #[SearchUsingFullText([
        ['firstname', 'lastname', 'internal_clientnumber']
    ])]
    public function toSearchableArray(): array
    {
        return [
            'firstname'          => $this->firstname,
            'lastname'           => $this->lastname,
            'internal_clientnumber' => $this->internal_clientnumber,
            'generated_fullname' => $this->firstname . $this->lastname,
            'email'              => $this->email,
        ];
    }
SELECT *
FROM `client`
WHERE (MATCH (`client`.`firstname`,
              `client`.`lastname`,
              `client`.`internal_clientnumber`) AGAINST ('muster' IN NATURAL LANGUAGE MODE)
       OR `client`.`generated_fullname` LIKE '%muster%'
       OR `client`.`email` LIKE '%muster%')

Arguments

🟢 Pro

Similarly to linked previous requests I mistakenly assumed that the existence of the SearchUsingFullText attribute meant that compound full-text indexes are supported.

🔴 Against

Like mentioned already in #833 (comment) - it can be done using the default query builder.

I felt like the cost of this change is not so heavy compared to already existing similar feature request tickets and that it would be a nice "completion" of the SearchUsingFullText behaviour.

Tests

Did not add any - I believe there are non for the attributes to limit the required migrations? ( 🤔 ) Correct me if I'm wrong on that.

What has been done - I hooked up my local scout package to a bigger internal project and was testing the behaviour and queries like that. There are no breaking changes so the existing tests should run through.

Copy link

github-actions bot commented Nov 1, 2024

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@jpedryc jpedryc changed the title Tmp Feature Request: Compound Full-Text Indexes Nov 1, 2024
@jpedryc jpedryc marked this pull request as ready for review November 1, 2024 02:41
Previously there was no support for compound full-text index search
capabilities. This extension handles these indexes by allowing
sub-arrays within the existing `SearchUsingFullText` attribute.

Example:
Assuming we have a 2 column full-text index ("col_a", "col_b")
and a single full-text index "col_b":

`#SearchUsingFullText([["col_a", "col_b"], "col_c"])`

will result in a SQL query like:

  MATCH (`t`.`col_a`, `t`.`col_b`) AGAINST ('term' IN <NLM>)
  OR MATCH (`t`.`col_c`) AGAINST ('term' IN <NLM>)

NOTE: Previous usages of this attribute will be not affected by this
      change.
@jpedryc
Copy link
Author

jpedryc commented Nov 1, 2024

The array method felt nicer, but alternatively could be also rebuild to comma-separated version like:

#[SearchUsingFullText(["col_a, col_b", "col_c"])

"col_a", "col_b" are within the compound full-text index in that case.

@taylorotwell
Copy link
Member

I am closing this pull request because it lacks sufficient explanation, tests, or both. It is difficult for us to merge pull requests without these things because the change may introduce breaking changes to the framework.

Feel free to re-submit your change with a thorough explanation of the feature and tests - integration tests are preferred over unit tests. Please include it's benefit to end users; the reasons it does not break any existing features; how it makes building web applications easier, etc.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants