Skip to content
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

🎁 Adding indices to Bulkrax Tables #813

Merged
merged 11 commits into from
Jun 14, 2023
Merged

Conversation

jeremyf
Copy link
Contributor

@jeremyf jeremyf commented Jun 8, 2023

Note: this is a best guess of what could use indexing. The guess is based on my recent querying.

  • add_index :bulkrax_entries, :identifier :: because the identifier is an important concept for an import.

  • add_index :bulkrax_entries, :type :: because filtering on the type of an entry is important.

  • add_index :bulkrax_entries, [:importerexporter_id, :importerexporter_type], name: 'bulkrax_entries_importerexporter_idx :: this is a foreign key for the entries relationship to the importer/exporter.

  • add_index :bulkrax_pending_relationships, :parent_id :: a foreign key

  • add_index :bulkrax_pending_relationships, :child_id :: a foreign key

  • add_index :bulkrax_statuses, [:statusable_id, :statusable_type], name: 'bulkrax_statuses_statusable_idx' :: a foreign key

  • add_index :bulkrax_statuses, [:runnable_id, :runnable_type], name: 'bulkrax_statuses_runnable_idx' :: a foreign key

  • add_index :bulkrax_statuses, :error_class :: when querying what's in error, this index is helpful; also one used for grouping.

Note: this is a best guess of what could use indexing.  The guess is
based on my recent querying.

- `add_index :bulkrax_entries, :identifier` :: because the identifier is
  an important concept for an import.
- `add_index :bulkrax_entries, :type` :: because filtering on the type
  of an entry is important.
- `add_index :bulkrax_entries, [:importerexporter_id,
  :importerexporter_type], name: 'bulkrax_entries_importerexporter_idx` :: this is a foreign key for the entries
  relationship to the importer/exporter.

- `add_index :bulkrax_pending_relationships, :parent_id` :: a foreign key
- `add_index :bulkrax_pending_relationships, :child_id`  :: a foreign key

- `add_index :bulkrax_statuses, [:statusable_id, :statusable_type],
  name: 'bulkrax_statuses_statusable_idx'` :: a foreign key
- `add_index :bulkrax_statuses, [:runnable_id, :runnable_type], name:
  'bulkrax_statuses_runnable_idx'` :: a foreign key
- `add_index :bulkrax_statuses, :error_class` :: when querying what's in
  error, this index is helpful; also one used for grouping.
@jeremyf jeremyf added the patch-ver for release notes label Jun 8, 2023
In downstream implementations, I'm seeing 1.4.0.  I'm seeing 1.5.0
failing in specs.
In specs, I was seeing the following error:

```
Psych::AliasesNotEnabled: Cannot load database configuration: Alias
parsing was not enabled. To enable it, pass `aliases: true` to
`Psych::load` or `Psych::safe_load`.
```

Given that we rarely touch the file, I see no benefit to using an alias
relative to the above error exception.
Looking at Adventist, which has a working Redis instance, I see that it
uses 4.2.5; however the newer version (v5) has the following error:

`ArgumentError: wrong number of arguments (given 1, expected 0)`; that
error is part of redis-client which is part of redis 5.0.6
First, this is well past end of life.  And second, I'm seeing the
following in the 2.6 build:

```
ArgumentError: wrong number of arguments (given 4, expected 1)
/home/runner/work/bulkrax/bulkrax/vendor/bundle/ruby/2.6.0/gems/psych-5.1.0/lib/psych.rb:322:in
`safe_load'
```

Not interested in fighting against that.
As this is for specs, I'm favoring that functionality/implementation.
I'm seeing the following in CI:

```
ArgumentError: wrong number of arguments (given 4, expected 1)
/home/runner/work/bulkrax/bulkrax/vendor/bundle/ruby/2.7.0/gems/psych-5.1.0/lib/psych.rb:322:in
`safe_load'
```
@@ -4,22 +4,23 @@
# Ensure the SQLite 3 gem is defined in your Gemfile
# gem 'sqlite3'
#
default: &default
Copy link
Contributor

Choose a reason for hiding this comment

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

was this not working? or do you just have a preference of explicitly stating the variables for each environment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was not working because of some Psych stuff.

@ShanaLMoore
Copy link
Contributor

consider removing coverage task

@ShanaLMoore ShanaLMoore merged commit 1b28da7 into main Jun 14, 2023
@ShanaLMoore ShanaLMoore deleted the adding-indices-to-tables branch June 14, 2023 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants