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/11/03 14:11:59 UTC
[GitHub] [lucene-solr] gerlowskija opened a new pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
gerlowskija opened a new pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056
# Description
Atomic Update 'remove' operations currently fail on several numeric fieldtypes when the "existing" doc hasn't been committed yet and is fetched from the UpdateLog.
# Solution
Modify logic for "remove" operation in AtomicUpdateDocumentMerger to check for comparable but different types (e.g. Long vs Integer).
# Tests
Manual testing for XML/JSON/javabin requests. Automated tests for Javabin and XML formats.
# 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.
- [ ] I have run `./gradlew check`.
- [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] gerlowskija merged pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
Posted by GitBox <gi...@apache.org>.
gerlowskija merged pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056
----------------------------------------------------------------
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] gerlowskija commented on pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056#issuecomment-724828898
Hey @munendrasn new code should address your concerns:
- handling for the string values that come through when requests are made using XML.
- handling for the "add-distinct" class of operations.
- additional tests to cover the two cases above.
I ended up _not_ going with the approach you suggested above of using `toNativeType()` to convert the whole list of "original" values, primarily for the performance concern I mentioned above. I'm not in love with the code I've got now though - the type checks are pretty ugly. So maybe I'll flip on this after a bit more consideration.
----------------------------------------------------------------
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] gerlowskija commented on pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056#issuecomment-725554469
Deciding to keep the implementation as-is for now. We can revisit if other type-related bugs pop up, or anyone has insight into what the downstream effects of the bulk `toNativeType` call might be on uncommitted docs.
One last note, in writing the tests for this PR I noticed that we have an abundance of test files for atomic updates, with a lot of overlap among them. I didn't want to dive into sorting those out here, but I'm going to file a follow up ticket for cutting out some of the overlap among these.
----------------------------------------------------------------
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] gerlowskija commented on pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056#issuecomment-722539440
I thought of doing a `toNativeType` conversion on the "original" Collection, but didn't like how it changed SolrInputDocument values for field-vals supposedly unaffected by the operation (e.g. an atomic remove of 1 of 5 values would change all 5 to native-types). I was leery of what problems that might cause downstream.
But if there are input edge cases I'm missing in the current iteration, maybe it's worth revisiting that approach. I'll take a look at that and some of the other things you mentioned (esp. add-distinct reproduction).
----------------------------------------------------------------
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] munendrasn commented on pull request #2056: SOLR-14971: Handle atomic-removes on uncommitted docs
Posted by GitBox <gi...@apache.org>.
munendrasn commented on pull request #2056:
URL: https://github.com/apache/lucene-solr/pull/2056#issuecomment-721133321
> As PR is open, sharing the comment here instead of JIRA
I was able to reproduce the issue with the script shared in JIRA. Regarding the approach, I agree SolrInputDocument returning field values in the actual type even for uncommitted docs is a better solution (approach 1) but currently not sure of the impact
I was able to reproduce the same issue with `add-distinct` atomic operation too. `add`, `set` doesn't have this problem. I think `removeregex` might have the same issue but I'm assuming this operation would be used with string/text fields, so probably not an issue? 🤔
Coming to the PR,
* This doesn't handle the case when the original values are passed as string. Suppose `values: ["2", "3"]`, then uncommitted doc would contain values as a string (This can happen if the users are using XML format (older version of solrJ sends update docs in XML format, not sure of the latest). In such cases, it will still not work
* would the PR support date fields type too?
I think to handle these cases, we need to convert the existing values to native types using
Something like this but not sure of performance overhead in case of large fields
```java
Collection<Object> original = existingField.getValues();
original.stream().map(val -> sf.getType().toNativeType(object)).collect(Collectors.toList())
```
----------------------------------------------------------------
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