perf(imports): Bulk import CSV transactions #1962

2025-03-08T10:07:19+08:00

Joelute commented

2025-03-08 10:07:19 +08:00

(Migrated from github.com)

Bulk import transactions from CSV instead of importing each row at a time resulting in a optimization in performance and faster imports

Entries	Before: (s)	After: (s)
1000	5.7	2.2
10000	93.9	16.4
100000	2083.7	203.8

Fixes: #1846.

Bulk import transactions from CSV instead of importing each row at a time resulting in a optimization in performance and faster imports | Entries | Before: (s) | After: (s) | |:---:|:---:|:---:| | 1000 | 5.7 | 2.2 | | 10000 | 93.9 | 16.4 | | 100000 | 2083.7 | 203.8| Fixes: #1846.

zachgoll (Migrated from github.com) reviewed 2025-03-10 21:33:55 +08:00

app/models/transaction_import.rb

zachgoll (Migrated from github.com) commented

2025-03-10 21:31:10 +08:00

Since we're using Postgres, I think we could use the recursive option here to avoid doing all the matching logic and still leverage the benefits of activerecord-import:

entries = rows.map do |row|
  # Build the entry
  Account::Entry.new(
    account: mapped_account,
    # other attributes
    entryable: Account::Transaction.new(category: category, tags: tags)
  )
end

Account::Entry.import!(entries, recursive: true)

https://github.com/zdennis/activerecord-import?tab=readme-ov-file#recursive

Since we're using Postgres, I think we could use the `recursive` option here to avoid doing all the matching logic and still leverage the benefits of activerecord-import: ```rb entries = rows.map do |row| # Build the entry Account::Entry.new( account: mapped_account, # other attributes entryable: Account::Transaction.new(category: category, tags: tags) ) end Account::Entry.import!(entries, recursive: true) ``` https://github.com/zdennis/activerecord-import?tab=readme-ov-file#recursive

Joelute (Migrated from github.com) reviewed 2025-03-12 09:27:30 +08:00

app/models/transaction_import.rb

Joelute (Migrated from github.com) commented

2025-03-12 09:27:30 +08:00

I've been playing around with it and it doesn't seem like it works well with delegated types. Even with the recursive option, I get the error: "Validation failed: Entryable must exist". Rails is expecting the Entryable to already be in the database with an ID.

I've optimized the code a bit so that we are aren't matching between arrays.

I've been playing around with it and it doesn't seem like it works well with delegated types. Even with the `recursive` option, I get the error: "Validation failed: Entryable must exist". Rails is expecting the `Entryable` to already be in the database with an ID. I've optimized the code a bit so that we are aren't matching between arrays.

zachgoll (Migrated from github.com) reviewed 2025-03-18 01:30:03 +08:00

app/models/transaction_import.rb

zachgoll (Migrated from github.com) commented

2025-03-18 01:30:02 +08:00

@Joelute looks like activerecord-import's recursive option doesn't handle belongs_to relationships; only has_many and has_one. I got things working by flipping the association:

  def import!
    transaction do
      mappings.each(&:create_mappable!)

      transactions = rows.map do |row|
        mapped_account = if account
          account
        else
          mappings.accounts.mappable_for(row.account)
        end

        category = mappings.categories.mappable_for(row.category)
        tags = row.tags_list.map { |tag| mappings.tags.mappable_for(tag) }.compact

        Account::Transaction.new(
          category: category,
          tags: tags,
          entry: Account::Entry.new(
            account: mapped_account,
            date: row.date_iso,
            amount: row.signed_amount,
            name: row.name,
            currency: row.currency,
            notes: row.notes,
            import: self
          )
        )
      end

      Account::Transaction.import!(transactions, recursive: true)
    end
  end

The main reason I'd like to use this recursive option is because I think it is the primary benefit we're getting from the additional gem. If we're not using it, I'd prefer to remove the gem and use insert_all for the bulk updates.

@Joelute looks like `activerecord-import`'s `recursive` option doesn't handle `belongs_to` relationships; only `has_many` and `has_one`. I got things working by flipping the association: ```rb def import! transaction do mappings.each(&:create_mappable!) transactions = rows.map do |row| mapped_account = if account account else mappings.accounts.mappable_for(row.account) end category = mappings.categories.mappable_for(row.category) tags = row.tags_list.map { |tag| mappings.tags.mappable_for(tag) }.compact Account::Transaction.new( category: category, tags: tags, entry: Account::Entry.new( account: mapped_account, date: row.date_iso, amount: row.signed_amount, name: row.name, currency: row.currency, notes: row.notes, import: self ) ) end Account::Transaction.import!(transactions, recursive: true) end end ``` The main reason I'd like to use this `recursive` option is because I think it is the _primary_ benefit we're getting from the additional gem. If we're not using it, I'd prefer to remove the gem and use `insert_all` for the bulk updates.

Joelute (Migrated from github.com) reviewed 2025-03-18 03:35:57 +08:00

app/models/transaction_import.rb

Joelute (Migrated from github.com) commented

2025-03-18 03:35:57 +08:00

Interesting, I should take a look into how the recursive option works and the benefits of it. I've updated the PR to reflect those changes.

zachgoll (Migrated from github.com) approved these changes 2025-03-19 21:02:30 +08:00

zachgoll (Migrated from github.com) left a comment

Looks good! Thanks for tackling this.

Joelute commented

2025-03-21 10:46:28 +08:00

(Migrated from github.com)

Just rebased the branch with the new changes.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#1962