Skip to content

sources.beancount: Correct type for links and tags columns #242

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

andreasgerstmayr
Copy link
Contributor

Currently, [...] GROUP BY links throws the following exception:

beanquery.compiler.CompilationError: GROUP-BY a non-hashable type is not supported: "Column(name='links')"

because set is not hashable. frozenset is hashable, and can be used for the links and tags which don't need to be mutable.

Currently, `[...] GROUP BY links` throws the following exception:

    beanquery.compiler.CompilationError: GROUP-BY a non-hashable type is not supported: "Column(name='links')"

because `set` is not hashable. `frozenset` is hashable, and can be used
for the `links` and `tags` which don't need to be mutable.
@dnicolodi dnicolodi changed the title Support GROUP BY for links and tags sources.beancount: Correct type for links and tags columns Feb 4, 2025
@dnicolodi
Copy link
Collaborator

Thanks for working on this. I trusted the old type annotation in Beancount and deduced that these fields are returned as sets, not frozen sets. Newer Beancount has the right annotations. This should definitely be fixed, however, the proposed patch is not enough: there are other things that need to be adjusted to support frozen sets. The first that comes to mind is support to render columns with frozenset data type (without that the rendering code falls back to the rendering of generic objects, which is quite ugly for frozen sets).

It is not strictly related to the PR itself, but if the added test case is an example of why you need this functionality, I think there is a better way of doing what you are doing:

2010-02-21 open Assets:AccountsReceivable:Doctor20100221

2010-02-21 * "Doctor appointment"
    Assets:AccountsReceivable:Doctor20100221   1000.00 USD
    Assets:Bank:Checking

2010-02-22 * "Insurance reimbursement"
    Assets:AccountsReceivable:Doctor20100221   -100.00 USD
    Assets:Bank:Checking

2010-03-22 * "Insurance reimbursement"
    Assets:AccountsReceivable:Doctor20100221   -900.00 USD
    Assets:Bank:Checking

2010-03-22 close Assets:AccountsReceivable:Doctor20100221

with a query like:

SELECT
  FIRST(date) as date, 
  FIRST(payee) AS payee, 
  FIRST(narration) AS narration,
  SUM(position) AS balance
WHERE account ~ 'Assets:AccountsReceivable:'
GROUP BY account
HAVING not empty(sum(position))
ORDER BY balance DESC

@andreasgerstmayr
Copy link
Contributor Author

Thanks for working on this. I trusted the old type annotation in Beancount and deduced that these fields are returned as sets, not frozen sets. Newer Beancount has the right annotations. This should definitely be fixed, however, the proposed patch is not enough: there are other things that need to be adjusted to support frozen sets. The first that comes to mind is support to render columns with frozenset data type (without that the rendering code falls back to the rendering of generic objects, which is quite ugly for frozen sets).

Thanks! I'll update the PR and also add a new test for the rendering.

It is not strictly related to the PR itself, but if the added test case is an example of why you need this functionality, I think there is a better way of doing what you are doing:

I agree, it feels a bit like a hack (I was surprised that grouping by links works in beancount.query), but I like to simplicity of it, as I don't have to open/close a new account for every reimbursement. The auto_accounts plugin would help with this, but then I don't catch typos in account names (e.g. a misspelled Expenses:Groceries etc.).

@dnicolodi
Copy link
Collaborator

I agree, it feels a bit like a hack (I was surprised that grouping by links works in beancount.query), but I like to simplicity of it, as I don't have to open/close a new account for every reimbursement. The auto_accounts plugin would help with this, but then I don't catch typos in account names (e.g. a misspelled Expenses:Groceries etc.).

The problem of using links in this way is that it makes it impossible to use them for anything else as adding links to a transaction break the GROUP BY but there isn't a mechanism in place to make sure that additional links are not added. Furthermore, there is no check on the set of links is consistent (ie that there are no typos). Finally, it works kind of ok for tracking reimbursements where you have one expense and a few reimbursements transactions. However, it breaks down quite badly if you have a single transaction reimbursing expenses occurred in multiple transactions, or for which only a part is reimbursable. All these problems are not there using a dedicated reimbursement account per reimbursement request. I use this system to track the reimbursement of my travel expenses and it scales well. I think the cost of the open and close entries is worth the advantages.

@andreasgerstmayr
Copy link
Contributor Author

Good points, I'll consider it in the future.

I added a frozenset renderer (inherited from the set renderer) to the PR. Do I need to update anything else?

@andreasgerstmayr
Copy link
Contributor Author

Actually, you got me convinced. I just refactored my entire ledger (with autobean-refactor), and added a customized version of the auto_accounts plugin to only auto-open Assets:AccountsReceivable accounts. Best of both worlds.

Anyway, I think the PR is still valid, possibly there are other use cases for grouping by links or tags.

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.

2 participants