Skip to content

Text Index Support #4081

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 6 commits into from
Sep 2, 2017
Merged

Text Index Support #4081

merged 6 commits into from
Sep 2, 2017

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Aug 4, 2017

Attempts to add an text index if $text exists in query.

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #4081 into master will decrease coverage by 0.01%.
The diff coverage is 72.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4081      +/-   ##
==========================================
- Coverage    90.7%   90.68%   -0.02%     
==========================================
  Files         116      116              
  Lines        7916     7926      +10     
==========================================
+ Hits         7180     7188       +8     
- Misses        736      738       +2
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 91.78% <72.72%> (-1.11%) ⬇️
src/Controllers/DatabaseController.js 94.16% <0%> (-0.21%) ⬇️
src/RestWrite.js 93.16% <0%> (+0.18%) ⬆️
src/Adapters/Auth/instagram.js 94.11% <0%> (+5.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92d51de...f1a13c9. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #4081 into master will decrease coverage by 0.01%.
The diff coverage is 90.9%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4081      +/-   ##
==========================================
- Coverage   90.84%   90.82%   -0.02%     
==========================================
  Files         116      116              
  Lines        7993     8003      +10     
==========================================
+ Hits         7261     7269       +8     
- Misses        732      734       +2
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 92.75% <90.9%> (-0.14%) ⬇️
src/RestWrite.js 93% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bace67...6b1d903. Read the comment docs.

};
return this.createIndex(className, index)
.catch((error) => {
if (error.code === 85) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of throwing error I could delete all current text indexes, add the new one and retry.

@montymxb
Copy link
Contributor

@dplewis just wanted to check in and see where this stands. If I understand correctly all that's missing is the ability to handle compound indexes?

@dplewis
Copy link
Member Author

dplewis commented Aug 18, 2017

@montymxb It depends on the level of support we want.

One field per ParseObject
This is what I'm currently aiming for in the PR. In the case of trying to use more than one. I can send an error message telling the user to delete all current text indexes before using another field. Or have the server delete all text indexes and add the new field.

Two or more fields per ParseObject
Similar to one field except a level of managing the text indexes and creating compound indexes out of it. Note you can only have one compound index.

A) createIndex({subject: "text"})
B) Want to add {comments: "text"}
C) delete Index created in A
D) createIndex({subject: "text", comment:"text"})
E) Want to add {reviews:text}
F) delete index created in D
G) createIndex({subject: "text", comments: "text", reviews: "text"})
H) All indexes must be tracked or queried to rebuild the compound index

Edit: we also have to keep in mind that some users may still manage their own indexes and those indexes may conflict with the indexes created by the server.

Thoughts?

@montymxb
Copy link
Contributor

montymxb commented Sep 1, 2017

@dplewis gotcha. Personally I am not entirely familiar with index management in mongodb. I will be looking into this more to bring myself up to speed so I can more effectively assist in this regard, especially if we choose to work with compound indexes in the future.

In reference to what you've shown it would be ideal to have 2 or more, but I think the simplest route of 1 index will suffice for now. Given this will be a relatively new feature we can worry about enhancing it for multi-index per collection if interests perks up for that. Everyone will have been managing indexes by hand until now so we'll see how this goes.

I think returning an error informing the user they should clear existing indexes would be a good start as well. We don't support managing them (yet), just creating them as needed and if able to do so. This way if we have an index conflict it shouldn't be deleting anything that may have been created by hand outside of parse.

Just got back from a vacation, sorry for the delay on the follow up!

@dplewis
Copy link
Member Author

dplewis commented Sep 2, 2017

@flovilmart Thoughts?

@flovilmart
Copy link
Contributor

I like that the index gets automatically created like the geopoints and for sure more complex indexing may be needed by the user himself.

@dplewis
Copy link
Member Author

dplewis commented Sep 2, 2017

@flovilmart Is this good to go? Also whats happening with travis?

@montymxb
Copy link
Contributor

montymxb commented Sep 2, 2017

@dplewis restarted the cancelled job for you, everything looks good in the test cases and code 👍.

Copy link
Contributor

@montymxb montymxb left a comment

Choose a reason for hiding this comment

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

Changes look good, tests look good.

@flovilmart
Copy link
Contributor

Awesome! Merging!

@flovilmart flovilmart merged commit 5aafc93 into parse-community:master Sep 2, 2017
@dplewis dplewis deleted the text-index branch September 2, 2017 19:51
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.

3 participants