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 2021/01/07 20:36:45 UTC
[GitHub] [lucene-solr] kwatters opened a new pull request #2185: LUCENE-9659 inequality support in payload check query
kwatters opened a new pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185
<!--
_(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
This PR is in support for LUCENE-9659 it updates the SpanPayloadCheckQuery to support inequality operations on string/integer/floating point encoded payload values.
# Solution
This creates a factory that implements the operation logic for decoding the payload and applying the inequality test to a reference payload. Existing behavior is preserved if the operation is null or "eq".
# Tests
A unit test was added for this new functionality to test string payloads greater than / less than / greater than or equal / less than or equal.
# Checklist
Please review the following and check all that apply:
- [ ] 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.
- [ ] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] 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)
- [ ] I have developed this patch against the `master` branch.
- [ ] I have run `./gradlew check`.
- [ ] 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] kwatters commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
kwatters commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770430584
With a little more investigation into a lambda approach for the compare method, (which on it's surface seems like a nice fit.) it got a bit messy because each subclass would have to define the code to encode the bytesRef into the native int/float/string objects. @dsmiley if you feel really strongly about it, I can see about refactoring it.
----------------------------------------------------------------
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 #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#discussion_r567486167
##########
File path: lucene/queries/src/java/org/apache/lucene/queries/payloads/PayloadMatcherFactory.java
##########
@@ -0,0 +1,249 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.queries.payloads;
+
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import org.apache.lucene.queries.payloads.SpanPayloadCheckQuery.MatchOperation;
+import org.apache.lucene.queries.payloads.SpanPayloadCheckQuery.PayloadType;
+import org.apache.lucene.util.ArrayUtil;
+import org.apache.lucene.util.BytesRef;
+
+/**
+ * Creates a payload matcher object based on a payload type and an operation. PayloadTypes of
+ * INT,FLOAT, or STRING are supported. Inequality operations are supported.
+ */
+public class PayloadMatcherFactory {
+
+ private static final HashMap<PayloadType, HashMap<MatchOperation, PayloadMatcher>>
+ payloadCheckerOpTypeMap;
+
+ static {
+ payloadCheckerOpTypeMap = new HashMap<PayloadType, HashMap<MatchOperation, PayloadMatcher>>();
+ // ints
+ HashMap<MatchOperation, PayloadMatcher> intCheckers =
Review comment:
Can you use `java.util.EnumMap` for these maps?
----------------------------------------------------------------
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] iverase commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-780653993
I think this commit has broken some tests:
```
gradlew test --tests TestPayloadCheckQuery.testInequalityPayloadChecks -Dtests.seed=892AF6C5F1C8E77D -Dtests.slow=true -Dtests.badapples=true -Dtests.locale=en-HK -Dtests.timezone=America/St_Thomas -Dtests.asserts=true -Dtests.file.encoding=UTF-8
```
The error looks like:
```
16:44:57 > java.lang.AssertionError: SpanPayloadCheckQuery(spanNear([fifty, five], 0, true), payloadRef: pos: 0;pos: 1;, payloadType:STRING;, operation:GT;) expected:<[55, 155, 255, 355, 455, 555, 655, 755, 855, 955, 1055, 1155, 1255, 1355, 1455, 1555, 1655, 1755, 1855, 1955]> but was:<[155, 255, 355, 455, 555, 655, 755, 855, 955, 1055, 1155, 1255, 1355, 1455, 1555, 1655, 1755, 1855, 1955]>
16:44:57 > at __randomizedtesting.SeedInfo.seed([892AF6C5F1C8E77D:91EA52CB633FFD94]:0)
16:44:57 > at org.junit.Assert.fail(Assert.java:89)
16:44:57 > at org.junit.Assert.failNotEquals(Assert.java:835)
16:44:57 > at org.junit.Assert.assertEquals(Assert.java:120)
16:44:57 > at org.apache.lucene.search.CheckHits.checkHits(CheckHits.java:157)
16:44:57 > at org.apache.lucene.queries.payloads.TestPayloadCheckQuery.checkHits(TestPayloadCheckQuery.java:100)
16:44:57 > at org.apache.lucene.queries.payloads.TestPayloadCheckQuery.testInequalityPayloadChecks(TestPayloadCheckQuery.java:270)
16:44:57 > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
16:44:57 > at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
16:44:57 > at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
```
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-780294457
Oh need to add a changes entry...
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-780758842
That's a change *sigh* Looks like a c&p error while trying to satisfy spotless based on my local history
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-775152136
I'm sorry for the delay, I've got a bit backlogged. Will comment soon.
----------------------------------------------------------------
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 #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770932208
BTW I saw your relationship Gus but sometimes I enjoy reviewing some code too :-).
----------------------------------------------------------------
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] gus-asf merged pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf merged pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-771044438
> BTW I saw your relationship Gus but sometimes I enjoy reviewing some code too :-).
:) always very welcome of course. I'm slightly irritated with myself for not wrangling the tools correctly to get the notifications, but more review is always excellent.
----------------------------------------------------------------
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] kwatters commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
kwatters commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770899018
Thanks Dave, How about something like the following for the changes.txt
LUCENE-9659 - Add support for inequality operations in the PayloadCheckQuery (Kevin Watters, Gus Heck)
----------------------------------------------------------------
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] kwatters commented on a change in pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
kwatters commented on a change in pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#discussion_r567530588
##########
File path: lucene/queries/src/java/org/apache/lucene/queries/payloads/PayloadMatcherFactory.java
##########
@@ -0,0 +1,249 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.lucene.queries.payloads;
+
+import java.nio.charset.StandardCharsets;
+import java.util.HashMap;
+import org.apache.lucene.queries.payloads.SpanPayloadCheckQuery.MatchOperation;
+import org.apache.lucene.queries.payloads.SpanPayloadCheckQuery.PayloadType;
+import org.apache.lucene.util.ArrayUtil;
+import org.apache.lucene.util.BytesRef;
+
+/**
+ * Creates a payload matcher object based on a payload type and an operation. PayloadTypes of
+ * INT,FLOAT, or STRING are supported. Inequality operations are supported.
+ */
+public class PayloadMatcherFactory {
+
+ private static final HashMap<PayloadType, HashMap<MatchOperation, PayloadMatcher>>
+ payloadCheckerOpTypeMap;
+
+ static {
+ payloadCheckerOpTypeMap = new HashMap<PayloadType, HashMap<MatchOperation, PayloadMatcher>>();
+ // ints
+ HashMap<MatchOperation, PayloadMatcher> intCheckers =
Review comment:
Now that MatchOperation is an enum, that makes a lot of sense. I just made that update. I looked into using a lambda, but I think it got messy because I'm actually using inheritance in the PayloadMatcherFactory for the various subclasses to handle the various types.
----------------------------------------------------------------
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 #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770911912
I very welcome your code review as well Gus! Give it a look; be my guest!
----------------------------------------------------------------
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] iverase commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
iverase commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-780748800
Unfortunately precommit only checks style, it does not run the tests.
----------------------------------------------------------------
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] kwatters commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
kwatters commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770922102
When all is good with this change, I'll update the PR for SOLR-14787.
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-780743941
That's odd I ran precommit... and so did github.
----------------------------------------------------------------
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] gus-asf commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
gus-asf commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770889210
Hmm, I had been going to look at this but I guess I should have assigned myself on the PR as well, I didn't get notified of any of this commentary until Dave re-assigned to himself in Jira. Dave, did you consider this in light of the related solr ticket discussions on https://issues.apache.org/jira/browse/SOLR-14787 ?
----------------------------------------------------------------
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] kwatters commented on pull request #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
kwatters commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-770416890
I simplified the equals operator, and as a result pulled out the MatchOperation into an enum. I think this is probably cleaner. Additional test cases were added to the testEquality tests in the TestPayloadCheckQuery to handle a few extra cases there weren't exercised before.
As for the lambda's , where were you suggesting to use a lambda? Perhaps, that's a refactor for another time, I think the performance of the current impl is pretty tight as it is.
----------------------------------------------------------------
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 #2185: LUCENE-9659 inequality support in payload check query
Posted by GitBox <gi...@apache.org>.
dsmiley commented on pull request #2185:
URL: https://github.com/apache/lucene-solr/pull/2185#issuecomment-774875316
@gus-asf , are you happy with the PR as well? I've been waiting on your input, or maybe you want to commit it.
----------------------------------------------------------------
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