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

FullText Search in laravel-oci8 #800

Open
wants to merge 21 commits into
base: 10.x
Choose a base branch
from

Conversation

jonas-elias
Copy link

@jonas-elias jonas-elias commented Aug 3, 2023

Title 😎
Hello, this pull request implements the "whereFullText()" method for laravel/oci8.

Description 🤯
The implementation utilizes the CONTAINS Oracle feature to perform a text search using the ctxsys.context index. To facilitate the creation of the index, a migration method named "fullText(nameColumn)" is included.

Additional notes 🤨
I have made modifications to the "testDropAllTables()" method to incorporate a constraint for secondary tables created by Oracle. This change prevents an error when dropping secondary tables whose parent tables have text reference indexes, and ensures the proper disposal of secondary tables (Oracle's automatic cascade). The relevant column "SECONDARY" is described in the documentation (references 3 and 4).

Running tests 🚀
./vendor/phpunit/phpunit/phpunit --filter=testWhereFullText
./vendor/phpunit/phpunit/phpunit --filter=testOrWhereFullText

References 🖥

  1. Oracle Documentation - CONTAINS
  2. Oracle Base - Full Text Indexing Using Oracle Text
  3. Oracle Documentation - ALL_TABLES
  4. Oracle Documentation - STATVIEW

Please review the changes, and if everything looks good, consider merging the pull request. Thank you 😁!

Copy link
Contributor

@hpacleb hpacleb left a comment

Choose a reason for hiding this comment

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

A few notes and maybe add an orWhereFullText test as well.

src/Oci8/Query/Grammars/OracleGrammar.php Outdated Show resolved Hide resolved
src/Oci8/Query/Grammars/OracleGrammar.php Outdated Show resolved Hide resolved
@jonas-elias
Copy link
Author

A few notes and maybe add an orWhereFullText test as well.

Added!! Thanks for remember!

@jonas-elias
Copy link
Author

Relevant issues have been addressed... Now it is possible to utilize the "orWhereFullText()" and the option "labelSearch" not need passing in the "$options" of the "whereFullText()" method.

Copy link
Contributor

@hpacleb hpacleb left a comment

Choose a reason for hiding this comment

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

Awesome, tested locally and everything seems to work as expected. Looks good to me.

@yajra
Copy link
Owner

yajra commented Aug 5, 2023

Everything looks correct except for multiple column fullText:

$table->fullText(['name', 'username', 'email'], 'users_fulltext_search');

Error

Error Code    : 29851
Error Message : ORA-29851: cannot build a domain index on more than one column
Position      : 51
Statement     : create index users_fulltext_search on users (name, username, email) indextype is ctxsys.context parameters ('sync(on commit)')
Bindings      : []
 (Connection: oracle, SQL: create index users_fulltext_search on users (name, username, email) indextype is ctxsys.context parameters ('sync(on commit)'))

Tested on Oracle Database 21c Express Edition Release 21.0.0.0.0 - Production

@@ -487,6 +487,17 @@ public function testAddingIndex()
$this->assertEquals('create index baz on users ( foo, bar )', $statements[0]);
}

public function testAddingFullTextIndex()
Copy link
Owner

Choose a reason for hiding this comment

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

SQL generated is as expected but doesn't work when executed on actual DB migration.

Copy link
Author

Choose a reason for hiding this comment

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

omg.. multiple columns index in oracle is implemented with ctx_ddl preferences 😅

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, ctx_ddl is something new to me 😅. Do I need some special setup for this work?

Will also try to research this. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Is new for me too 😅, but i researched about this, is simple to use.

For create multiple columns index, is necessary execute grant CTX_DDL to owner.

The code create preference "idx" and set configuration for columns "FIRSTNAME" and "LASTNAME", attribuiting in creation of index "firstname_multiple":

BEGIN
    ctx_ddl.create_preference('idx', 'MULTI_COLUMN_DATASTORE');
    ctx_ddl.set_attribute('idx', 'COLUMNS', 'FIRSTNAME, LASTNAME');
    EXECUTE IMMEDIATE 'create index firstname_multiple ON "USERS" ("FIRSTNAME") INDEXTYPE IS CTXSYS.CONTEXT PARAMETERS (''DATASTORE idx SYNC(ON COMMIT)'')';
END;

Is possible search if content contains in column "LASTNAME" by:
SELECT * FROM USERS u WHERE CONTAINS(FIRSTNAME, 'content_lastname') > 0

Summarized implementation of multiple full text index columns in oracle 😎

References:
https://docs.oracle.com/en/database/oracle/oracle-database/19/ccref/CTX_DDL-package.html#GUID-0F7C39E8-E44A-421C-B40D-3B3578B507E9

Copy link
Author

Choose a reason for hiding this comment

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

I fixed this bug and implemented ctx_ddl preferences to create multi_columns_index oracle feature. In method compileFullText(), i implemented one index to each column, enabling the search of multi column preference. If you have any suggestions, don't hesitate to help me 😅. Thanks!

Copy link
Author

Choose a reason for hiding this comment

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

Hey @hpacleb, you have a new suggestions? 😁

@yajra yajra requested a review from hpacleb August 12, 2023 02:10
@yajra
Copy link
Owner

yajra commented Dec 14, 2023

@jonas-elias thank you for the updates. We will review it as soon as we can.

Ping @hpacleb ^_^

@richardcardoso198
Copy link

Any updates on this?

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.

4 participants