Skip to content

DEV: Add category_users converter and importer steps #33367

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 1 commit into
base: main
Choose a base branch
from

Conversation

s3lase
Copy link
Contributor

@s3lase s3lase commented Jun 26, 2025

This adds converter(Discourse-only, for now) and importer steps for category_users

This adds converter(Discourse-only, for now) and importer steps for `category_users`
@github-actions github-actions bot added the migrations-tooling PR which includes changes to migrations tooling label Jun 26, 2025
@s3lase s3lase requested a review from gschlager June 27, 2025 00:23
Comment on lines +34 to +39
@existing_category_users = Hash.new { |h, k| h[k] = Set.new }

@discourse_db
.query_array("SELECT category_id, user_id FROM category_users WHERE user_id >= 0")
.each { |row| @existing_category_users[row[0]].add(row[1]) }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn’t too happy with the string concatenation approach I used here and here, so I did some testing and benchmarking. This new approach is about 1.8x faster and avoids allocating new strings for each row during lookups.

I think we should elevate this pattern it to a macro, but there are a few things I'd like to work through first:

  • Should this be a new macro? Or should we extend requires_map to support this via a config?
  • What about requires_set? I'm not sure, it feels like it would add unnecessary overhead won't be intuitive in it's current form
  • Should it just return a set of arrays?
  • What happens to hierarchically scoped keys when there are more than two columns?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
migrations-tooling PR which includes changes to migrations tooling
Development

Successfully merging this pull request may close these issues.

1 participant