Use solr's graph query builder for nested collection queries #5850

cjcolvar · 2022-09-09T21:09:53Z

This can replace the expensive nested indexing using samvera-nesting_indexer and all of the special handling for it.

Note: the graph query builder does not work with multi-sharded solr instances

All nesting indexing has been removed along with unused methods/parameters. Deprecation notices will be added to a future backport PR to 3.x.

spec/services/hyrax/collections/nested_collection_query_service_spec.rb

cjcolvar · 2022-09-09T21:25:56Z

spec/services/hyrax/collections/nested_collection_query_service_spec.rb

-            expect(subject.map(&:id)).to contain_exactly(coll_a.id, another.id)
+            expect(subject.map(&:id)).to contain_exactly(another.id)


I think this test was incorrect before. If A -> B -> C -> D -> E then I wouldn't expect A to be valid as an "available parent" for C because it already is. I believe this is actually prevented by the validator if you attempt to do this in the UI.

I believe in the current implementation, C can have A as an ancestor through B and as a direct parent. So

A -> B -> C -> D -> E
A -> C -> D -> E

at the same time. BUT I do think that works more by accident than by design.

Allowing E to be a direct child of C and indirect child of C was intentional but perhaps from a theoretical stand point instead of anything concrete.

FWIW I tested again locally and this works for User Collections, but when I attempt to do this with a collection type that doesn't allow multiple membership, I get an error:

Hyrax::SingleMembershipError in Hyrax::Dashboard::NestCollectionsController#create_relationship_within Error: You have specified more than one of the same single-membership collection type (type: Nesting only, collections: C and D)

I'll look into restoring this functionality.

I added support for this back in with dfc2f09

cjcolvar · 2022-09-09T21:27:16Z

spec/services/hyrax/collections/nested_collection_query_service_spec.rb

+      # Limit no longer applies so it will always return true
+      it 'returns true' do
+        expect(subject).to eq true


We could use solr's maxDepth parameter to the graph query parser, but that feels like more work than it is worth.

that makes sense to me.

If you exceed it there should just be a message that says "you have delved too deep. the Balrog comes"

cjcolvar · 2022-09-12T15:52:05Z

This should also fix #5596, #5595, and #5492.

dlpierce · 2022-09-12T17:46:38Z

This sounds great! Has there been any performance testing with meaningful/large hyrax data sets? Would perhaps using the traversalFilter graph query option provide any benefit?

no-reply · 2022-09-12T18:52:10Z

this looks great to me.

since the performance of the old implementation is extremely bad, it seems so likely that the performance will be improved. i'd like to suggest we move forward with this and leave any testing for post merge.

i'd also like to look at a limited backport of this, keeping the old indexing behavior in place for 3.x by default, but introducing the new query behavior. i'll poke at this later today.

cjcolvar · 2022-09-12T19:02:34Z

@dlpierce I've done some very limited manual testing and haven't seen any issues with solr. I'm not sure if the traversalFilter is useful for our use case or not. I'd be curious about performance as nesting depth scales but I'd suspect solr would handle most normal cases easily.

jeremyf · 2022-09-12T21:13:16Z

app/forms/hyrax/forms/dashboard/nest_collection_form.rb

-          if nestable?(parent)
-            nesting_within_maximum_depth
-          else
+          unless nestable?(parent)


How are we guarding circular nesting? (e.g. A has member B has member C has member A)?

According to the documentation, the graph query parser is "cyclic aware".

on a current Hyrax, we allow circular nesting.

I think it is being handled by the parent_and_child_can_be_nested validation on the form.

@no-reply Can you give an example of currently allowed/working circular nesting?

i was able to create this situation trivially in the UI.

I wasn't able to reproduce this in nurax-dev. Can you please post the exact steps that allowed this for you? I'd like to reproduce this in nurax-dev then test the same steps with this PR to see how the behavior differs.

no-reply · 2022-09-12T21:55:26Z

What's the right way to document that Hyrax 4.0 doesn't support sharded SolrCloud? it seems like we need to write that down somewhere, even if it's unlikely to impact any current users.

no-reply · 2022-09-12T23:55:55Z

app/forms/hyrax/forms/dashboard/nest_collection_form.rb

-        # needed to make the determination are too expensive to do for every possible
-        # collection, so we only test for this situation prior to saving the new
-        # relationship.
-        def nesting_within_maximum_depth


i'm trying to write a commit message for the backport of this change---which no-ops this validation if the solr graph query is configured. but i'm a bit stuck and realizing i'm not entirely clear on why we're removing this. can you help me @cjcolvar?

I assumed that the max depth was only needed to limit the amount of nested reindexing to undertake. Now that the indexing isn't necessary the technical limit seems like solr's graph query parser's efficiency. If there is a functional reason for having a limit (and making it adopter configurable) then I could restore the depth limit enforcement with a new implementation. The configuration of the max depth was also on the Samvera::NestingIndexer so we'd have to make a new config in Hyrax for it.

thanks! super helpful

orangewolf · 2022-09-13T04:14:02Z

What's the right way to document that Hyrax 4.0 doesn't support sharded SolrCloud? it seems like we need to write that down somewhere, even if it's unlikely to impact any current users.

I would think a note on this page https://samvera.github.io/service-stack.html (which could stand some expansion and more prominant linking in the readme, but that's a different ticket) and a comment in app/search_builders/hyrax/dashboard/nested_collections_search_builder.rb when we talk about the graph search. (because that's where things will break if you multishard).

In a perfect world, we would throw an exception or send a notice or something on startup if multiple shards are detected, but I don't think we even mandate solr cloud usage, so I'm not sure how that API call would be reliable and again, I feel like "Check if Solr configuration is sane on start up" is a whole new ticket.

This can replace the expensive nested indexing using samvera-nesting_indexer

…nts and children

dlpierce

My testing of the style of solr query used here worked well and seemed to be speedy with lots of collections. 👍

cjcolvar

* backport #5850: limited/guarded backport of query portion Co-authored-by: Chris Colvard <chris.colvard@gmail.com> Co-authored-by: Rob Kaufman <rob@notch8.com>

cjcolvar force-pushed the solr_graph branch from c3b1e98 to 078d56a Compare September 9, 2022 21:19

cjcolvar commented Sep 9, 2022

View reviewed changes

spec/services/hyrax/collections/nested_collection_query_service_spec.rb Outdated Show resolved Hide resolved

cjcolvar commented Sep 9, 2022

View reviewed changes

cjcolvar force-pushed the solr_graph branch from d9fad90 to 05e081f Compare September 9, 2022 22:55

cjcolvar marked this pull request as ready for review September 9, 2022 23:09

cjcolvar mentioned this pull request Sep 12, 2022

Index member_of_collection_ids for PcdmCollection #5852

Merged

jeremyf reviewed Sep 12, 2022

View reviewed changes

no-reply pushed a commit that referenced this pull request Sep 12, 2022

backport #5850: limited/guarded backport of query portion

cef05a3

no-reply pushed a commit that referenced this pull request Sep 12, 2022

backport #5850: limited/guarded backport of query portion

02f5a4c

no-reply reviewed Sep 12, 2022

View reviewed changes

jlhardes added this to the 4.x Series milestone Sep 28, 2022

jlhardes added the valkyrization label Sep 28, 2022

cjcolvar added 4 commits September 28, 2022 16:05

Use solr's graph query builder for nested collection queries

f23358d

This can replace the expensive nested indexing using samvera-nesting_indexer

Remove all code related to nested indexing

6772fe2

Further cleanup and removal of Nested collection indexing

a39da92

Restore ability for collections to be direct as well as indirect pare…

78a6098

…nts and children

cjcolvar force-pushed the solr_graph branch from dfc2f09 to 78a6098 Compare September 28, 2022 20:09

cjcolvar requested review from jeremyf, orangewolf, no-reply and dlpierce September 28, 2022 20:38

dlpierce approved these changes Sep 28, 2022

View reviewed changes

dlpierce merged commit 9131b9c into main Sep 30, 2022

dlpierce deleted the solr_graph branch September 30, 2022 20:35

dlpierce added the notes-major Release Notes: Potentially breaking changes label Oct 12, 2022

laritakr mentioned this pull request Oct 28, 2022

Spike: Is the graph indexer a part of hyrax upgrade in UTK? scientist-softserv/utk-hyku#154

Closed

orangewolf pushed a commit that referenced this pull request Nov 16, 2022

backport #5850: limited/guarded backport of query portion

5378340

cjcolvar commented Nov 16, 2022

View reviewed changes

orangewolf added a commit that referenced this pull request Nov 18, 2022

Backport #5850 (#5858)

298075b

* backport #5850: limited/guarded backport of query portion Co-authored-by: Chris Colvard <chris.colvard@gmail.com> Co-authored-by: Rob Kaufman <rob@notch8.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use solr's graph query builder for nested collection queries #5850

Use solr's graph query builder for nested collection queries #5850

cjcolvar commented Sep 9, 2022 •

edited

Loading

cjcolvar Sep 9, 2022

orangewolf Sep 9, 2022

jeremyf Sep 12, 2022

cjcolvar Sep 13, 2022

cjcolvar Sep 13, 2022

cjcolvar Sep 13, 2022

cjcolvar Sep 9, 2022

orangewolf Sep 9, 2022

cjcolvar commented Sep 12, 2022

dlpierce commented Sep 12, 2022

no-reply commented Sep 12, 2022

cjcolvar commented Sep 12, 2022

jeremyf Sep 12, 2022

dlpierce Sep 12, 2022

no-reply Sep 12, 2022

cjcolvar Sep 13, 2022

cjcolvar Sep 13, 2022

no-reply Sep 13, 2022

cjcolvar Sep 21, 2022

no-reply commented Sep 12, 2022

no-reply Sep 12, 2022

cjcolvar Sep 13, 2022

no-reply Sep 13, 2022

orangewolf commented Sep 13, 2022

dlpierce left a comment

cjcolvar left a comment •

edited

Loading

		expect(subject.map(&:id)).to contain_exactly(coll_a.id, another.id)
		expect(subject.map(&:id)).to contain_exactly(another.id)

Use solr's graph query builder for nested collection queries #5850

Use solr's graph query builder for nested collection queries #5850

Conversation

cjcolvar commented Sep 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcolvar commented Sep 12, 2022

dlpierce commented Sep 12, 2022

no-reply commented Sep 12, 2022

cjcolvar commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

no-reply commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orangewolf commented Sep 13, 2022

dlpierce left a comment

Choose a reason for hiding this comment

cjcolvar left a comment • edited Loading

Choose a reason for hiding this comment

cjcolvar commented Sep 9, 2022 •

edited

Loading

cjcolvar left a comment •

edited

Loading