singlestore-labs / singlestoredb-laravel-driver

The official SingleStore Laravel driver.
https://github.com/singlestore-labs/singlestore-laravel-driver
Apache License 2.0
223 stars 22 forks source link

Add support for whereFullText #30

Closed fgilio closed 1 year ago

fgilio commented 1 year ago

Follow up from https://github.com/singlestore-labs/singlestoredb-laravel-driver/pull/9

This adds support for using Laravel's whereFullText(...) method for full text search.

Relevant docs pages:

This would also be the first step to add support for HIGHLIGHT.


WIP:

carlsverre commented 1 year ago

re: your WIP questions

I need to see if we need to do something more to support orWhereFullText What would the behavior of orWhereFullText be?

I'm not sure we need to catch the exception and throw a custom one, SingleStore already provides an easily understandable error message I think the exception you are throwing here is much clearer than the one thrown by the engine.

guywarner commented 1 year ago

@carlsverre we solved this issue in a little bit different way.

We added it the QueryBuilder - https://github.com/guywarner/laravel-singlestore-fulltext/blob/main/src/Database/SingleStoreQueryBuilder.php

This then just piggybacks off of the ->whereRaw() this allows it to be included in the $wheres[] and Laravel can do its normal magic of building the query. To handle the issue of orWhereText it'd be just as simple as call ->orWhereRaw()

fgilio commented 1 year ago

@carlsverre we solved this issue in a little bit different way.

We added it the QueryBuilder - https://github.com/guywarner/laravel-singlestore-fulltext/blob/main/src/Database/SingleStoreQueryBuilder.php

This then just piggybacks off of the ->whereRaw() this allows it to be included in the $wheres[] and Laravel can do its normal magic of building the query. To handle the issue of orWhereText it'd be just as simple as call ->orWhereRaw()

I think this solution is more ergonomic for Laravel developers, as it allows you to continue using Laravel's native Full Text Where method.

Do you see any other opportunity for improvement in this implementation?

fgilio commented 1 year ago

@carlsverre I think this is finally ready.

I added assertions for the actual results, with which I had a blocker last time and now managed to solve with the OPTIMIZE TABLE <table_name> FLUSH trick.

fgilio commented 1 year ago

There are still 2 outstanding questions and one outstanding change request from my original review. Also tests are not passing.

Completely missed those, I'll take a look

fgilio commented 1 year ago

Just a couple minor tweaks - then let's ship it! Thank you!

I think we're ready now!

fgilio commented 1 year ago

Awesome! We'll be able to remove our custom implementation now