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

SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...} #1514

Closed
wants to merge 7 commits into from

Conversation

danmfox
Copy link
Contributor

@danmfox danmfox commented May 13, 2020

Description

Updates the cross-collection join query in #976 based on the feedback in SOLR-13749. In that ticket there was a preference to consolidate the cross-collection join functionality into the existing join query parser, rather than creating a new separate query parser.

Solution

This PR integrates the cross-collection join query parser into the existing join query parser plugin. The syntax for a cross-collection join changes from {!xcjf ...} to {!join method=crossCollection ...}. The arguments that could previously be set on the XCJF query parser plugin can now be set on the join query parser plugin.

Tests

The XCJFQueryTest class has been updated to use the new query syntax (and renamed to CrossCollectionJoinQueryTest).

Checklist

Please review the following and check all that apply:

  • I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
  • I have created a Jira issue and added the issue ID to my pull request title.
  • I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
  • I have developed this patch against the master branch.
  • I have run ant precommit and the appropriate test suite.
  • I have added tests for my changes.
  • I have added documentation for the Ref Guide (for Solr changes only).

@danmfox danmfox changed the title SOLR-13749: Change cross-collection join query syntax to {!join method=ccjoin ...} SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...} May 29, 2020
Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

I'm still looking for the defaulting logic so that a user doesn't even need to know about "crossCollection"; it'll just work.

if (args.get("solrUrlWhitelist") != null) {
//noinspection unchecked
for (String s : (List<String>) args.get("solrUrlWhitelist")) {
if (!StringUtils.isEmpty(s))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, really, but personally I'd just go with 1 line of code -- solrUrlWhiteList.addAll((List<String>) args.get("solrUrlWhitelist")) because the impact of an empty string here is safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me - I made this change while updating the property name.

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Missing ref-guide changes?

@dsmiley
Copy link
Contributor

dsmiley commented Jun 5, 2020

If someone uses join QParser with either no method or the default method=index and if fromIndex refers to a non-qualifying collection (sharded or not node-local), they'll get one of the errors printed from this method: org.apache.solr.search.join.ScoreJoinQParserPlugin#findLocalReplicaForFromIndex.

  • "SolrCloud join: multiple shards not yet supported " + fromIndex. I suggest rewording: To join with a sharded collection, use method=crossCollection.
  • "SolrCloud join: No active replicas for "+fromIndex+" found in node " + nodeName. I suggest rewording: To join with a collection that might not be co-located, use method=crossCollection

There is another case as well when the local replica is not in an Active state but lets ignore that.

Copy link
Contributor

@dsmiley dsmiley left a comment

Choose a reason for hiding this comment

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

Great; I think that's it! I'll merge this weekend or perhaps Monday.

@dsmiley
Copy link
Contributor

dsmiley commented Jun 14, 2020

@danmfox Still missing Ref Guide changes to account for the refactor

@dsmiley
Copy link
Contributor

dsmiley commented Jun 17, 2020

Last minute request: also please change solrUrlWhitelist to avoid this "whitelist" word (think current events) to, I propose "allowSolrUrls". There's a discussion going on now internally that probably should be public about this sort of thing. There's another patch to add a new similar thing for file system paths deliberately named "allowPaths", so that influenced my suggested new name here.

@dsmiley dsmiley closed this Jun 27, 2020
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.

4 participants