-
-
Notifications
You must be signed in to change notification settings - Fork 292
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: id as number #441
feat: id as number #441
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hi @H4ad ! Could you resolve the conflicts? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR seems ok. I left some comments. In particular, related to DocumentID: why do we need to support both, number or string?
return false | ||
} | ||
|
||
delete store.docs[id] | ||
delete store.docs[internalId] | ||
store.count-- | ||
|
||
return true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should remove also from sharedInternalDocumentStore
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we just remove it from sharedInternalDocumentStore.idToInternalId
, it's fast enough.
If we try to remove it from sharedInternalDocumentStore.internalIdToId
, it will be slower as sorter
.
What we can do is perform a cleanup of internalIdToId
on serialize.
What do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to make sure, so no need to remove the id from the sharedInternalDocumentStore
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No sorry, we should implement it.
For now, we don't care about the performance during the remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, we don't care about the performance during the remove.
Agreed. I'd dedicate a separate PR to this
@@ -0,0 +1,70 @@ | |||
import { Orama } from '../types.js'; | |||
|
|||
export type DocumentID = string | number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why allow both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to introduce less breaking change as possible (I also don't know what is public API and what is internal API).
Someone that changes the implementation of sorter
, index
, etc... will not need to modify their code in order to accept this change.
But if you want to go full breaking change mode, I can use InternalID
in every code (but still return the original ID on search).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok to introduce a little breaking change with this. Regarding the naming. Is this a documented or IndexId?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will bump Orama to v1.1.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I will change all the references for DocumentID
to InternalID
, and I will only use DocumentID
on getByID
and when we return the documents from search
.
Regarding the naming. Is this a documented or IndexId?
I didn't understand your question but DocumentID
is just an alias to reference the ID of the Document that was generated or was passed by the user, we you see this type, is referring to these two cases.
But I think we should add some documentation about it on Orama Docs, just to be clear about how we store IDs to be more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, understood you point. fine for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, amazing work!
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terrific job @H4ad. As always!
LGTM
/claim #426
This is my attempt to closes #426, in this implementation, I added a new component called
internalDocumentIDStore
which is responsible for keeping the internal IDs and also has a list to reverse those IDs.About the performance, to compare this change with the old behavior, here are the current stats for the old behavior:
And the new behavior is:
database-size.mjs
This is what looks like the current serialization: