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: add support for custom document id field #1884

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

prog8
Copy link

@prog8 prog8 commented Dec 4, 2023

With this iteration we keep consistency with current format of document identifiers. Auto-generated identifier is a hexadecimal string representation of binary identifier. In case of custom ID field we keep exactly the same format. This means a user has to make sure he sends document with custom ID field that follows this pattern. With the next iteration we can adjust the identifier so it doesn't require ID to be hex string.

@@ -716,8 +716,8 @@ func (e *Engine) upsertDocuments(ctx context.Context, sqlTx *sql.SQLTx, collecti

provisionedDocID, docIDProvisioned := doc.Fields[docIDFieldName]
if docIDProvisioned {
if isInsert {
return 0, nil, fmt.Errorf("%w: field (%s) should NOT be specified when inserting a document", ErrIllegalArguments, docIDFieldName)
if isInsert && docIDFieldName == DefaultDocumentIDField {
Copy link
Contributor

Choose a reason for hiding this comment

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

if I read correctly, if inserted document does not have custom id, then immudb will autogenerate one. I would block that, as it is doing something by the user behind the scenes, so something that I would not expect when the user explicitly specifies that document id is provided externally. I.e. even if ids are just hex encoded strings at the end, they might have more meaning from the user perspective.

Copy link
Author

Choose a reason for hiding this comment

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

Indeed. In case of missing custom ID provided by the user it will be auto-generated. This is similar strategy for example Elasticsearch uses. If user doesn't specify an identifier it is automatically generated. Each approach has pros and cons of course. In #1755 I mentioned that maybe it's not worth giving a user flexibility to provide document DB with custom ID field just force the name of ID field but let user customize the value. In such a case it's even more important to have auto-generated _id if not provided or use the one from the user if available.

embedded/document/engine_test.go Show resolved Hide resolved
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.

None yet

2 participants