feat: add numeric collation for balance sorting and support sort parameter in RPC calls#129
feat: add numeric collation for balance sorting and support sort parameter in RPC calls#129TheCrazyGM wants to merge 3 commits intohive-engine:qafrom
Conversation
…meter in RPC calls
…strings correctly
| } | ||
|
|
||
| // Add numeric collation for balance/stake sorting (both are numeric strings) | ||
| const collation = ind.some(idx => idx.index === 'balance' || idx.index === 'stake') |
There was a problem hiding this comment.
I believe these are probably the only fields that would be popular, but any of the others could be added as needed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
|
That could be quite helpful. Good job Michael. |
| } | ||
|
|
||
| // Add numeric collation for balance/stake sorting (both are numeric strings) | ||
| const collation = ind.some(idx => idx.index === 'balance' || idx.index === 'stake') |
There was a problem hiding this comment.
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.
| // 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 |
There was a problem hiding this comment.
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.
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.
You probably want a table that has a lot more entries, like nft.
Testtime curl -s -X POST http://localhost:5000/contracts -H "Content-Type: application/json" --data '{
"jsonrpc": "2.0",
"id": 1,
"method": "find",
"params": {
"contract": "nft",
"table": "STARinstances",
"query": {"ownedBy":"u"},
"sort": {"field": "account", "order": "desc"},
"limit": 10
}
}'Response{
"jsonrpc": "2.0",
"id": 1,
"result": [
{
"_id": 1128384,
"account": "wilson112000",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "EnergyBoost1",
"class": "energyboost",
"stats": "0,0,0,0"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 2099507,
"account": "santiagolecuona",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "EnergyBoost1",
"class": "energyboost",
"stats": "0,0,0,0"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 2381250,
"account": "ruviksan",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "R221 PT 100 Bass Head",
"class": "instrument",
"stats": "0,50,0,0"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 2743030,
"account": "n1t0",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "R237 Ivespino",
"class": "people",
"stats": "125,3,75,2"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 2463713,
"account": "marcos1977",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "R223 Mint Kit",
"class": "instrument",
"stats": "0,50,0,0"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 981739,
"account": "luke-wtp",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "EnergyBoost1",
"class": "energyboost",
"stats": "0,0,0,0"
},
"previousAccount": "stewball",
"previousOwnedBy": "u"
},
{
"_id": 986482,
"account": "luke-wtp",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "66 Stewie",
"class": "people",
"stats": "5,0,10,0"
},
"previousAccount": "stewball",
"previousOwnedBy": "u"
},
{
"_id": 3336889,
"account": "koko556",
"soulBound": false,
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "S44 Old Blud",
"class": "people",
"stats": "100,1,100,1"
},
"previousAccount": "robkingdom",
"previousOwnedBy": "u"
},
{
"_id": 178322,
"account": "juancarlosqp",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "i6 Mid Range Acoustic",
"class": "instrument",
"stats": "0,10,0,0"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
},
{
"_id": 1585024,
"account": "juancarlosqp",
"ownedBy": "u",
"lockedTokens": {},
"properties": {
"type": "R189 Roman",
"class": "people",
"stats": "125,3,75,2"
},
"previousAccount": "ivespino",
"previousOwnedBy": "u"
}
]
}ResultsIt took almost 30 seconds and put my mongo instance at 100% CPU. (one core) SuggestionsPut limits on what tables you can query with sort params? Or add an index to balance and stake and put the check back allowing only those two to be sorted? |
|
I think it's worth noting that the same api call on the nft table, without sort: time curl -s -X POST http://localhost:5000/contracts -H "Content-Type: application/json" --data '{
"jsonrpc": "2.0",
"id": 1,
"method": "find",
"params": {
"contract": "nft",
"table": "STARinstances",
"query": {"ownedBy":"u"},
"limit": 10
}
}'came back with a very similar response time, so I don't think it added much more overhead than I thought.
But, I've also added a catch in there to only allow sort on fields that are a) indexed or b) specifically balance or stake in the tokens contract. I don't know if this is the direction you want to take it. |
I think very likely what we need is just a way to enable the feature and allow node operators to choose. Also, the reason why the query is slow is probably the index is ['account', 'ownedBy'] and likely you'd want just 'ownedBy' directly as an index. To improve the queries alongside this change, we should set up the indexes on the fly, but again, leave that to node operator. |
Add ability to sort by balance / stake / etc.
API Call
Output as expected:
No index needed, using: