-
-
Notifications
You must be signed in to change notification settings - Fork 103
WIP: Compute balances in database #247
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?
Conversation
This part no longer relies on the GroupBalances table
Uses a prisma typedSQL query
Only the `typedSQL` files are committed. The client is excluded from eslint.
Can you try again now? Turns out I wasn't up to date on the migrations and for some reason when the column is |
1000 users in 101 groups with 10k expenses each
Type Hash for faster lookup with =
I've updated the seed file to create 1000 users and 101 groups with 10k expenses each. The largest group has 30 members all the others have 10. It should give us a better understanding of the performance. It does take quite a while to create all the expenses but it's a temporary change anyway. If we merge we can just revert the file completely. |
Fantastic work, thank you 🙌 |
No problem at all. Take your time. Better not to rush things :) |
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.
Very sorry for taking so long with the review. I had to take care of other stuff as well as more pressing current issues. I requested some changes that would minimize this PR further as I believe such risky changes should be as easy to review as possible. I ran some initial tests on my machine and it is looking good, so I would like to proceed with this, but only after they have been resolved and the branch gets rebased onto main. Furthermore, if you could give me write access to your fork, I will implement friend balances as well :)
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.
The original seed
script is useful for local dev work, while this one takes a long time to execute. Please move it to a separate file
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 plan was to only use this as and intermediate script and revert to the original before we merge. If you think it's worth keeping around I'll move it to a separate script.
const sortByIds = (a: getAllBalancesForGroup.Result, b: getAllBalancesForGroup.Result) => { | ||
if (a.paidBy === b.paidBy) { | ||
return a.borrowedBy - b.borrowedBy; | ||
} | ||
return a.userId - b.userId; | ||
return a.paidBy - b.paidBy; |
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 would prefer to keep this PR as minimal as possible. As such, please keep the property names identical and create a type GroupBalance = getAllBalancesForGroup.Result
alias
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 confirm, you want me to keep the existing firendId
including the typo?
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.
While I agree the naming even without the typo is quite unfortunate, I would prefer to make such a critical PR as small and easy to review as possible. We can think about another PR afterwards.
src/server/api/routers/group.ts
Outdated
const groupBalances = await ctx.db.$queryRawTyped(getGroupsWithBalances(ctx.session.user.id)); | ||
|
||
const _groups = groupBalances | ||
.map((b) => { | ||
return { | ||
id: b.id, | ||
name: b.name, | ||
}; | ||
}) | ||
.filter((obj, index, self) => index === self.findIndex((t) => t.id === obj.id)); | ||
|
||
const groupsWithBalances = _groups.map((group) => { | ||
const balancesForGroup: Record<string, bigint> = {}; | ||
groupBalances | ||
.filter((b) => { | ||
return b.id == group.id; | ||
}) | ||
.forEach((b) => { | ||
if (b.currency != null && b.balance != null) { | ||
balancesForGroup[b.currency] = b.balance; | ||
} | ||
}); |
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.
While we remain on the dev branch, I propose the following sanity check:
utilize both the legacy and new method and compare their results. When a mismatch is detected, throw an Error. We will remove it before creating a release.
Oh and one more thing, this change would break the current splitwise import functionality, as it does not import expenses as of now. #207 needs to be implemented first and as such I don't expect to go ahead with this PR for 1.5 release |
No problem. More time to polish. |
This is really good work! Calculating it based on the expenses rather then the balance table is amazing! 👏 |
Thanks for the ping. I'm of course happy to finish this. I'll start by merging in all the changes and fixing the conflicts. Shouldn't take more than a day or two until I have something up to date and working. |
Okay nice, since I would like to include my changes as well in this PR and have some thoughts I would like to share, would you like to connect on Discord to chat more conveniently? |
I've fixed the conflicts and properly formatted the SQL files. So far everything still seems to work as intended. The check currently fails because Github Actions doesn't allow to change the command for the postgres container to load the pg_cron extension. The easiest fix would probably be if you could update the custom postgres image to contain the |
Yeah, I think that would help. I'll email you my username. |
Good work! Looking forward to this change. |
@FriesischScott I've sent you a friend request. I am now working on an extensive seeding script with |
Good plan! |
The build error seems to stem from the fact you don't provide startup commands for cron. See the repo compose files for reference. |
@FriesischScott The seed script has been merged :) The next step would be to implement checks. Currently we still create balances, which also prevents us from inserting expenses in parallel as all the transactions create deadlocks. I would propose writing some consistency checks, that would compare as many cases of query calculated balances to the table ones as possible, preferably all of them. Once that is done, we can plan next steps, as the situation with direct user-user balances requires us to think through the transition from current Total balances to user-user ones. |
As discussed in #234 the goal of this PR is to drop the separate balance tables for users and groups and instead compute the balances from all expenses in the database.
I agree with @krokosik that we need to be certain we can handle the required loads for large instances with lots of expenses but I think postgresql will be up for the task.
This is a list of the required balances:
here I've implemented the second and third and I think @krokosik has started working on the first.
Because
prisma
can't handle the necessary computations through it's javascript interface I've chosen to usetypedSQL
to write native SQL queries and still benifit from prisma's typing capabilities.One minor downside is that
typedSQL
currently requires a live database connection to generate the types. However withprisma
7 the location of the generated client will move out ofnode_modules
and we will have to set an output directory like this.We could then commit the generated sql types in the
sql
subfolder of the prisma client and only generate the regular client during build. This should already be possible now (only in prisma 7 it's mandatory) and I'm working on setting it up.