From b1392bec9d2238a3c641b83511551526e4f219cf Mon Sep 17 00:00:00 2001 From: Jon Bratseth Date: Wed, 22 Jan 2020 13:37:32 +0100 Subject: Better select grouping parsing --- .../java/com/yahoo/prelude/query/NearItem.java | 8 +-- .../com/yahoo/search/dispatch/rpc/RpcClient.java | 3 +- .../java/com/yahoo/search/query/SelectParser.java | 72 +++++++++++++++------- .../query/profile/QueryProfileProperties.java | 2 +- .../parser/GroupingParserBenchmarkTest.java | 2 +- .../java/com/yahoo/search/test/QueryTestCase.java | 17 +---- .../test/java/com/yahoo/select/SelectTestCase.java | 9 ++- 7 files changed, 67 insertions(+), 46 deletions(-) (limited to 'container-search') diff --git a/container-search/src/main/java/com/yahoo/prelude/query/NearItem.java b/container-search/src/main/java/com/yahoo/prelude/query/NearItem.java index 69131b6c690..56554e14d01 100644 --- a/container-search/src/main/java/com/yahoo/prelude/query/NearItem.java +++ b/container-search/src/main/java/com/yahoo/prelude/query/NearItem.java @@ -28,17 +28,15 @@ public class NearItem extends CompositeItem { /** * Creates a near item with a limit to the distance between the words. * - * @param distance the number of word position which may separate - * the words for this near item to match + * @param distance the maximum position difference between the words which should be counted as a match */ public NearItem(int distance) { setDistance(distance); } public void setDistance(int distance) { - if (distance < 0) { - throw new IllegalArgumentException("Can not use negative distance '" + distance + "'."); - } + if (distance < 0) + throw new IllegalArgumentException("Can not use negative distance " + distance); this.distance = distance; } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcClient.java b/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcClient.java index 05ce6d50493..a2821892358 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcClient.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/rpc/RpcClient.java @@ -22,6 +22,7 @@ import java.util.List; * @author bratseth */ class RpcClient implements Client { + private final Supervisor supervisor; public RpcClient(int transportThreads) { @@ -66,7 +67,7 @@ class RpcClient implements Client { @Override public void request(String rpcMethod, CompressionType compression, int uncompressedLength, byte[] compressedPayload, - ResponseReceiver responseReceiver, double timeoutSeconds) { + ResponseReceiver responseReceiver, double timeoutSeconds) { Request request = new Request(rpcMethod); request.parameters().add(new Int8Value(compression.getCode())); request.parameters().add(new Int32Value(uncompressedLength)); diff --git a/container-search/src/main/java/com/yahoo/search/query/SelectParser.java b/container-search/src/main/java/com/yahoo/search/query/SelectParser.java index c654edda6f5..ae50756331c 100644 --- a/container-search/src/main/java/com/yahoo/search/query/SelectParser.java +++ b/container-search/src/main/java/com/yahoo/search/query/SelectParser.java @@ -45,6 +45,7 @@ import com.yahoo.search.yql.VespaGroupingStep; import com.yahoo.slime.ArrayTraverser; import com.yahoo.slime.Inspector; import com.yahoo.slime.ObjectTraverser; +import com.yahoo.slime.Type; import com.yahoo.vespa.config.SlimeUtils; import java.nio.charset.StandardCharsets; import java.util.ArrayList; @@ -159,7 +160,7 @@ public class SelectParser implements Parser { return new QueryTree(root); } - private Item walkJson(Inspector inspector){ + private Item walkJson(Inspector inspector) { Item[] item = {null}; inspector.traverse((ObjectTraverser) (key, value) -> { String type = (FUNCTION_CALLS.contains(key)) ? CALL : key; @@ -197,8 +198,8 @@ public class SelectParser implements Parser { public List getGroupingSteps(String grouping){ List groupingSteps = new ArrayList<>(); - List groupingOperations = getOperations(grouping); - for (String groupingString : groupingOperations){ + List groupingOperations = toGroupingRequests(grouping); + for (String groupingString : groupingOperations) { GroupingOperation groupingOperation = GroupingOperation.fromString(groupingString); VespaGroupingStep groupingStep = new VespaGroupingStep(groupingOperation); groupingSteps.add(groupingStep); @@ -206,24 +207,51 @@ public class SelectParser implements Parser { return groupingSteps; } - private List getOperations(String grouping) { - List operations = new ArrayList<>(); - Inspector inspector = SlimeUtils.jsonToSlime(grouping.getBytes()).get(); - if (inspector.field("error_message").valid()){ + /** Translates a list of grouping requests on JSON form to a list in the grouping language form */ + private List toGroupingRequests(String groupingJson) { + Inspector inspector = SlimeUtils.jsonToSlime(groupingJson.getBytes()).get(); + if (inspector.field("error_message").valid()) { throw new QueryException("Illegal query: " + inspector.field("error_message").asString() + " at: '" + new String(inspector.field("offending_input").asData(), StandardCharsets.UTF_8) + "'"); } - inspector.traverse( (ArrayTraverser) (key, value) -> { - String groupingString = value.toString(); - groupingString = groupingString.replace(" ", "").replace("\"", "").replace("\'", "").replace(":{", "(").replace(":", "(").replace("}", ")").replace(",", ")"); - groupingString = groupingString.substring(1, groupingString.length()); - operations.add(groupingString); - }); - + List operations = new ArrayList<>(); + inspector.traverse((ArrayTraverser) (__, item) -> operations.add(toGroupingRequest(item))); return operations; } + private String toGroupingRequest(Inspector groupingJson) { + StringBuilder b = new StringBuilder(); + toGroupingRequest(groupingJson, b); + return b.toString(); + } + + private void toGroupingRequest(Inspector groupingJson, StringBuilder b) { + switch (groupingJson.type()) { + case ARRAY: + groupingJson.traverse((ArrayTraverser) (index, item) -> { + toGroupingRequest(item, b); + if (index + 1 < groupingJson.entries()) + b.append(","); + }); + break; + case OBJECT: + groupingJson.traverse((ObjectTraverser) (name, object) -> { + b.append(name); + b.append("("); + toGroupingRequest(object, b); + b.append(") "); + }); + break; + case STRING: + b.append(groupingJson.asString()); + break; + default: + b.append(groupingJson.toString()); + break; + } + } + private Item buildFunctionCall(String key, Inspector value) { switch (key) { case WAND: @@ -250,7 +278,7 @@ public class SelectParser implements Parser { }); } else if (inspector.type() == OBJECT){ - if (inspector.field("children").valid()){ + if (inspector.field("children").valid()) { inspector.field("children").traverse((ArrayTraverser) (index, new_value) -> { item.addItem(walkJson(new_value)); }); @@ -259,22 +287,22 @@ public class SelectParser implements Parser { } } - private Inspector getChildren(Inspector inspector){ + private Inspector getChildren(Inspector inspector) { if (inspector.type() == ARRAY){ return inspector; } else if (inspector.type() == OBJECT){ - if (inspector.field("children").valid()){ + if (inspector.field("children").valid()) { return inspector.field("children"); } - if (inspector.field(1).valid()){ + if (inspector.field(1).valid()) { return inspector.field(1); } } return null; } - private HashMap childMap(Inspector inspector){ + private HashMap childMap(Inspector inspector) { HashMap children = new HashMap<>(); if (inspector.type() == ARRAY){ inspector.traverse((ArrayTraverser) (index, new_value) -> { @@ -291,14 +319,14 @@ public class SelectParser implements Parser { return children; } - private Inspector getAnnotations(Inspector inspector){ + private Inspector getAnnotations(Inspector inspector) { if (inspector.type() == OBJECT && inspector.field("attributes").valid()){ return inspector.field("attributes"); } return null; } - private HashMap getAnnotationMapFromAnnotationInspector(Inspector annotation){ + private HashMap getAnnotationMapFromAnnotationInspector(Inspector annotation) { HashMap attributes = new HashMap<>(); if (annotation.type() == OBJECT){ annotation.traverse((ObjectTraverser) (index, new_value) -> { @@ -308,7 +336,7 @@ public class SelectParser implements Parser { return attributes; } - private HashMap getAnnotationMap(Inspector inspector){ + private HashMap getAnnotationMap(Inspector inspector) { HashMap attributes = new HashMap<>(); if (inspector.type() == OBJECT && inspector.field("attributes").valid()){ inspector.field("attributes").traverse((ObjectTraverser) (index, new_value) -> { diff --git a/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java b/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java index ea79b10d779..b250560e2f3 100644 --- a/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java +++ b/container-search/src/main/java/com/yahoo/search/query/profile/QueryProfileProperties.java @@ -128,7 +128,7 @@ public class QueryProfileProperties extends Properties { } } catch (IllegalArgumentException e) { - throw new IllegalArgumentException("Could not set '" + name + "' to '" + value + "': " + e.getMessage()); // TODO: Nest instead + throw new IllegalArgumentException("Could not set '" + name + "' to '" + value + "'", e); } } diff --git a/container-search/src/test/java/com/yahoo/search/grouping/request/parser/GroupingParserBenchmarkTest.java b/container-search/src/test/java/com/yahoo/search/grouping/request/parser/GroupingParserBenchmarkTest.java index 326e37ede38..6d9c2218022 100644 --- a/container-search/src/test/java/com/yahoo/search/grouping/request/parser/GroupingParserBenchmarkTest.java +++ b/container-search/src/test/java/com/yahoo/search/grouping/request/parser/GroupingParserBenchmarkTest.java @@ -15,7 +15,7 @@ import java.util.concurrent.TimeUnit; */ public class GroupingParserBenchmarkTest { - private static final int NUM_RUNS = 10;//000; + private static final int NUM_RUNS = 10; private static final Map PREV_RESULTS = new LinkedHashMap<>(); static { diff --git a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java index 84565472820..ea34be5cf37 100644 --- a/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/test/QueryTestCase.java @@ -1,7 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.search.test; -import com.google.common.collect.ImmutableList; import com.yahoo.component.chain.Chain; import com.yahoo.language.Language; import com.yahoo.language.Linguistics; @@ -31,12 +30,10 @@ import com.yahoo.search.Query; import com.yahoo.search.Result; import com.yahoo.search.Searcher; import com.yahoo.search.grouping.GroupingQueryParser; -import com.yahoo.search.grouping.GroupingRequest; import com.yahoo.search.query.QueryTree; import com.yahoo.search.query.SessionId; import com.yahoo.search.query.profile.QueryProfile; import com.yahoo.search.query.profile.QueryProfileRegistry; -import com.yahoo.search.query.profile.compiled.CompiledQueryProfile; import com.yahoo.search.query.profile.compiled.CompiledQueryProfileRegistry; import com.yahoo.search.query.profile.types.QueryProfileType; import com.yahoo.search.result.Hit; @@ -48,24 +45,18 @@ import org.junit.Test; import java.io.UnsupportedEncodingException; import java.net.URLEncoder; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; -import static org.hamcrest.CoreMatchers.containsString; -import static org.hamcrest.CoreMatchers.is; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNotSame; import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertThat; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -240,10 +231,10 @@ public class QueryTestCase { @Test public void test_that_cloning_preserves_timeout() { Query original = new Query(); - original.setTimeout(9876l); + original.setTimeout(9876L); Query clone = original.clone(); - assertThat(clone.getTimeout(), is(9876l)); + assertEquals(9876L, clone.getTimeout()); } @Test @@ -297,9 +288,7 @@ public class QueryTestCase { fail("Above statement should throw"); } catch (QueryException e) { // As expected. - assertThat( - Exceptions.toMessageString(e), - containsString("Could not set 'timeout' to 'nalle': Error parsing 'nalle': Invalid number 'nalle'")); + assertTrue(Exceptions.toMessageString(e).contains("Could not set 'timeout' to 'nalle': Error parsing 'nalle': Invalid number 'nalle'")); } } diff --git a/container-search/src/test/java/com/yahoo/select/SelectTestCase.java b/container-search/src/test/java/com/yahoo/select/SelectTestCase.java index 261069ea1c3..96392a4f29b 100644 --- a/container-search/src/test/java/com/yahoo/select/SelectTestCase.java +++ b/container-search/src/test/java/com/yahoo/select/SelectTestCase.java @@ -652,7 +652,6 @@ public class SelectTestCase { assertGrouping(expected, parseGrouping(grouping)); } - @Test public void testMultipleGroupings() { String grouping = "[ { \"all\" : { \"group\" : \"a\", \"each\" : { \"output\" : \"count()\"}}}, { \"all\" : { \"group\" : \"b\", \"each\" : { \"output\" : \"count()\"}}} ]"; @@ -661,6 +660,13 @@ public class SelectTestCase { assertGrouping(expected, parseGrouping(grouping)); } + @Test + public void testGroupingWithPredefinedBuckets() { + String grouping = "[ { \"all\" : { \"group\" : { \"predefined\" : [ \"foo\", { \"bucket\": [1,2]}, { \"bucket\": [3,4]} ] } } } ]"; + String expected = "[[]all(group(predefined(foo, bucket[1, 2>, bucket[3, 4>)))]"; + assertGrouping(expected, parseGrouping(grouping)); + } + //------------------------------------------------------------------- Other tests @Test @@ -763,7 +769,6 @@ public class SelectTestCase { } private List parseGrouping(String grouping) { - return parser.getGroupingSteps(grouping); } -- cgit v1.2.3