You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/05/13 22:34:27 UTC
[GitHub] [lucene-solr] danmfox opened a new pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=ccjoin ...}
danmfox opened a new pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514
<!--
_(If you are a project committer then you may remove some/all of the following template.)_
Before creating a pull request, please file an issue in the ASF Jira system for Lucene or Solr:
* https://issues.apache.org/jira/projects/LUCENE
* https://issues.apache.org/jira/projects/SOLR
You will need to create an account in Jira in order to create an issue.
The title of the PR should reference the Jira issue number in the form:
* LUCENE-####: <short description of problem or changes>
* SOLR-####: <short description of problem or changes>
LUCENE and SOLR must be fully capitalized. A short description helps people scanning pull requests for items they can work on.
Properly referencing the issue in the title ensures that Jira is correctly updated with code review comments and commits. -->
# 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=ccjoin ...}`. 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:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [x] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `master` branch.
- [x] I have run `ant precommit` and the appropriate test suite.
- [x] I have added tests for my changes.
- [ ] I have added documentation for the [Ref Guide](https://github.com/apache/lucene-solr/tree/master/solr/solr-ref-guide) (for Solr changes only).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514#issuecomment-639863580
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley closed pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
dsmiley closed pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on a change in pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514#discussion_r433024112
##########
File path: solr/core/src/java/org/apache/solr/search/JoinQParserPlugin.java
##########
@@ -160,23 +176,40 @@ JoinParams parseJoin(QParser qparser) throws SyntaxError {
}
}
+ @Override
+ public void init(NamedList args) {
+ routerField = (String) args.get("routerField");
+ solrUrlWhitelist = new HashSet<>();
+ if (args.get("solrUrlWhitelist") != null) {
+ //noinspection unchecked
+ for (String s : (List<String>) args.get("solrUrlWhitelist")) {
+ if (!StringUtils.isEmpty(s))
Review comment:
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514#issuecomment-643826837
@danmfox Still missing Ref Guide changes to account for the refactor
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] danmfox commented on a change in pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
danmfox commented on a change in pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514#discussion_r441494859
##########
File path: solr/core/src/java/org/apache/solr/search/JoinQParserPlugin.java
##########
@@ -160,23 +176,40 @@ JoinParams parseJoin(QParser qparser) throws SyntaxError {
}
}
+ @Override
+ public void init(NamedList args) {
+ routerField = (String) args.get("routerField");
+ solrUrlWhitelist = new HashSet<>();
+ if (args.get("solrUrlWhitelist") != null) {
+ //noinspection unchecked
+ for (String s : (List<String>) args.get("solrUrlWhitelist")) {
+ if (!StringUtils.isEmpty(s))
Review comment:
Sounds good to me - I made this change while updating the property name.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
[GitHub] [lucene-solr] dsmiley commented on pull request #1514: SOLR-13749: Change cross-collection join query syntax to {!join method=crossCollection ...}
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #1514:
URL: https://github.com/apache/lucene-solr/pull/1514#issuecomment-645120762
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org