-
Notifications
You must be signed in to change notification settings - Fork 97
Fix Vote History sorting #5927
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: main
Are you sure you want to change the base?
Fix Vote History sorting #5927
Changes from all commits
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 |
|---|---|---|
|
|
@@ -42,7 +42,10 @@ trait DbVotesTxLogStoreQueryBuilder[TXE] | |
| TxLogQueries.SelectFromTxLogTableResult | ||
| ], TxLogQueries.SelectFromTxLogTableResult, Effect.Read] = { | ||
| val afterCondition = after match { | ||
| case Some(a) => Some(sql"""entry_number < $a""") | ||
| case Some(a) => | ||
| Some( | ||
| sql"""(record_time, entry_number) < ((select record_time from #$txLogTableName where store_id = $txLogStoreId and entry_number = $a), $a)""" | ||
|
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. did you check the plane for this? the SQL query looks weird to me.
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. +1 to commenting with the query plan. We just dropped the index on I would have expected that we do either:
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. We discussed this exactly (out of band) and decided we should use the (record_time, entry_number) in page token |
||
| ) | ||
| case None => None | ||
| } | ||
| val actionNameCondition = actionName match { | ||
|
|
@@ -92,7 +95,7 @@ trait DbVotesTxLogStoreQueryBuilder[TXE] | |
| txLogTableName, | ||
| txLogStoreId, | ||
| where = whereClause.toActionBuilder, | ||
| orderLimit = sql"""order by entry_number desc limit ${sqlLimit(limit)}""", | ||
| orderLimit = sql"""order by record_time desc, entry_number desc limit ${sqlLimit(limit)}""", | ||
| ) | ||
| } | ||
| } | ||
|
|
||
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.
@rautenrieth-da could you take a look if that's a correct way to remove unused indices? We didn't use
IndexAction.Dropbefore but AFAIU that's the wayThere 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.
The code looks good to me. Since this is the first time we're using it, we should check if it really works as intended. You won't trigger the code path in integration tests, you need to first deploy an old version of the app and then a version that has your PR merged. At the very least, check logs of CILR after the changes from this PR have been deployed there.