-
Notifications
You must be signed in to change notification settings - Fork 21
feat: add numeric collation for balance sorting and support sort parameter in RPC calls #129
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
base: qa
Are you sure you want to change the base?
Changes from 2 commits
4c0c32a
f25cd6c
93ed924
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -684,20 +684,28 @@ class Database { | |
| } | ||
| }); | ||
| } else { | ||
| // This can happen when creating a table and using find with index all in the same transaction | ||
| // and should be rare in production. Otherwise, contract code is asking for an index that does | ||
| // not exist. | ||
| log.info(`Index ${JSON.stringify(ind)} not available for ${finalTableName}`); // eslint-disable-line no-console | ||
| // Build sort array even when indexes don't exist | ||
| ind.forEach((el) => { | ||
| sort.push([el.index === '$loki' ? '_id' : el.index, el.descending === true ? 'desc' : 'asc']); | ||
| }); | ||
| log.info(`Index ${JSON.stringify(ind)} not available for ${finalTableName}, using collation for numeric sorting`); // eslint-disable-line no-console | ||
| } | ||
| if (sort.findIndex(el => el[0] === '_id') < 0) { | ||
| sort.push(['_id', 'asc']); | ||
| } | ||
|
|
||
| // Add numeric collation for balance/stake sorting (both are numeric strings) | ||
| const collation = ind.some(idx => idx.index === 'balance' || idx.index === 'stake') | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe these are probably the only fields that would be popular, but any of the others could be added as needed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that if you don't have an index with collation, this likely will stream everything and sort it, right? I suppose either way, if it is problematic, an index can be added directly to the table as a setup.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, without index it does pull the whole thing into memory to sort. I had included a script to add the indexes, but i decided against it, was trying to avoid touching the database directly.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, makes sense. But don't worry, index setup is not something that impacts hashes, and can make things better (or worse if you get too carried away with indexes i suppose, would impact writes i think) |
||
| ? { locale: "en_US", numericOrdering: true } | ||
| : undefined; | ||
|
|
||
| result = await tableData.find(EJSON.deserialize(query), { | ||
| limit: lim, | ||
| skip: off, | ||
| sort, | ||
| session: this.session, | ||
| projection: prj, | ||
| ...(collation && { collation }), | ||
| }).toArray(); | ||
|
|
||
| result = EJSON.serialize(result); | ||
|
|
||
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 be careful with this, because without an index on large collections it can be problematic to allow
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 have it running right now, can you think of a problematic query and I'll test. But I haven't noticed any impact, e.g the pizza token has 18K+ records and no delay.
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.
You probably want a table that has a lot more entries, like nft.