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

Feat inverted index for mariadb #22

Merged
merged 2 commits into from
Jun 8, 2021
Merged

Conversation

eldadfux
Copy link
Member

@eldadfux eldadfux commented Jun 3, 2021

No description provided.

@eldadfux eldadfux requested a review from kodumbeats June 3, 2021 11:00
Copy link
Contributor

@kodumbeats kodumbeats left a comment

Choose a reason for hiding this comment

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

Good work, just one section that could use a clarifying comment (in both find() and count())

@@ -526,7 +443,7 @@ public function find(string $collection, array $queries = [], int $limit = 25, i
$orders = [];

foreach($roles as &$role) {
$role = $this->getPDO()->quote($role, PDO::PARAM_STR);
$role = "+".str_replace('+', ' ', $role)."+";
Copy link
Contributor

Choose a reason for hiding this comment

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

I found why the + characters were added for boolean fulltext search, but I'm struggling to follow the logic as to how this line gets used in the permissions ternary operator:

str_replace('+', '"', $this->getPDO()->quote(implode(' ', $roles)))

Can we add some comments to assist future maintainers?

@kodumbeats
Copy link
Contributor

Performance gain is significant - below are tests at 100k records:

created.greater(), genre.equal()

roles    |      1   |   100    |    500   |   1000   |  2000
baseline | 0.5161 s | 0.5174 s | 0.6818 s | 0.8476 s | 1.0718 s
newindex | 0.0046 s | 0.0284 s | 0.1056 s | 0.2184 s | 0.4707 s

genre.equal(OR)

roles    |      1   |   100    |    500   |   1000   |  2000
baseline | 0.3802 s | 0.5840 s | 1.0252 s | 1.4866 s | 2.1468 s
newindex | 0.0021 s | 0.0141 s | 0.0793 s | 0.1867 s | 0.4308 s

views.greater()

roles    |      1   |   100    |    500   |   1000   |  2000
baseline | 0.3592 s | 0.6577 s | 1.4259 s | 2.3217 s | 3.9273 s
newindex | 0.0010 s | 0.0152 s | 0.0879 s | 0.1987 s | 0.4512 s

text.search()

roles    |      1   |   100    |    500   |   1000   |  2000
baseline | 3.0193 s | 3.1827 s | 3.7934 s | 4.6298 s | 6.0709 s
newindex | 0.2852 s | 0.0869 s | 0.1617 s | 0.2774 s | 0.5231 s

@eldadfux
Copy link
Member Author

eldadfux commented Jun 3, 2021

OK, this looks like a promising implementation so far, I think we should think if we need a few more tests to make sure the permissions functionality is working as expected. I will adapt the MySQL adapter to take advantage of native index capabilities and we can also compare the two.

@eldadfux eldadfux merged commit 7dbba11 into main Jun 8, 2021
This pull request was closed.
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