From da440710ed20bd5e45836c1bb4d31a19ba4eb41a Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Tue, 10 Dec 2024 16:10:40 +0100 Subject: [PATCH 1/2] Validate attribute vector data types for grouping requests. --- .../search/grouping/GroupingValidator.java | 36 +++++++++-- .../grouping/GroupingValidatorTestCase.java | 63 ++++++++++++++++++- 2 files changed, 91 insertions(+), 8 deletions(-) diff --git a/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java b/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java index 165469d59eef..d2ba4092b1c6 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java @@ -65,10 +65,34 @@ public Result search(Query query, Execution execution) { return execution.search(query); } - private void verifyHasAttribute(String attributeName) { - if (!attributes.containsKey(attributeName)) { + static private boolean isPrimitiveAttribute(AttributesConfig.Attribute attribute) { + var datatype = attribute.datatype(); + return datatype == AttributesConfig.Attribute.Datatype.INT8 || + datatype == AttributesConfig.Attribute.Datatype.INT16 || + datatype == AttributesConfig.Attribute.Datatype.INT32 || + datatype == AttributesConfig.Attribute.Datatype.INT64 || + datatype == AttributesConfig.Attribute.Datatype.STRING || + datatype == AttributesConfig.Attribute.Datatype.FLOAT || + datatype == AttributesConfig.Attribute.Datatype.DOUBLE; + } + + static private boolean isSingleRawAttribute(AttributesConfig.Attribute attribute) { + return attribute.datatype() == AttributesConfig.Attribute.Datatype.RAW && + attribute.collectiontype() == AttributesConfig.Attribute.Collectiontype.SINGLE; + } + + private void verifyHasAttribute(String attributeName, boolean allowSingleRawAttribute) { + var attribute = attributes.get(attributeName); + if (attribute == null) { throw new UnavailableAttributeException(clusterName, attributeName); } + if (isPrimitiveAttribute(attribute) || (isSingleRawAttribute(attribute) && allowSingleRawAttribute)) { + return; + } + throw new IllegalInputException("Grouping request references attribute '" + + attributeName + "' with unsupported data type '" + attribute.datatype() + + "' collection type '" + attribute.collectiontype() + "'" + + (allowSingleRawAttribute ? "" : " for map lookup")); } private void verifyCompatibleAttributeTypes(String keyAttributeName, @@ -92,14 +116,14 @@ private class MyVisitor implements ExpressionVisitor { @Override public void visitExpression(GroupingExpression exp) { if (exp instanceof AttributeMapLookupValue mapLookup) { - verifyHasAttribute(mapLookup.getKeyAttribute()); - verifyHasAttribute(mapLookup.getValueAttribute()); + verifyHasAttribute(mapLookup.getKeyAttribute(), false); + verifyHasAttribute(mapLookup.getValueAttribute(), false); if (mapLookup.hasKeySourceAttribute()) { - verifyHasAttribute(mapLookup.getKeySourceAttribute()); + verifyHasAttribute(mapLookup.getKeySourceAttribute(), false); verifyCompatibleAttributeTypes(mapLookup.getKeyAttribute(), mapLookup.getKeySourceAttribute()); } } else if (exp instanceof AttributeValue) { - verifyHasAttribute(((AttributeValue) exp).getAttributeName()); + verifyHasAttribute(((AttributeValue) exp).getAttributeName(), true); } } } diff --git a/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java index edce8112f21a..98a147a0fd4c 100644 --- a/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java @@ -145,11 +145,70 @@ void key_source_attribute_with_multi_value_collection_type_throws() { } } + private static void unsupported_attribute_type_throws(String name, AttributesConfig.Attribute.Datatype.Enum datatype) { + try { + var builder = new AttributesConfig.Builder(); + builder.attribute(new AttributesConfig.Attribute.Builder().name(name).datatype(datatype)); + validateGrouping(new AttributesConfig(builder), + "all(group(" + name + ") each(output(count())))"); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertEquals("Grouping request references attribute '" + name + "' " + + "with unsupported data type '" + datatype.toString() + "' collection type 'SINGLE'", + e.getMessage()); + } + } + + @Test + void tensor_attribute_throws() { + unsupported_attribute_type_throws("tensor", AttributesConfig.Attribute.Datatype.TENSOR); + } + + @Test + void predicate_attribute_throws() { + unsupported_attribute_type_throws("predicate", AttributesConfig.Attribute.Datatype.PREDICATE); + } + + @Test + void reference_attribute_throws() { + unsupported_attribute_type_throws("reference", AttributesConfig.Attribute.Datatype.REFERENCE); + } + + @Test + void raw_attribute_is_ok() { + var builder = new AttributesConfig.Builder(); + builder.attribute(new AttributesConfig.Attribute.Builder().name("raw") + .datatype(AttributesConfig.Attribute.Datatype.RAW)); + validateGrouping(new AttributesConfig(builder), + "all(group(raw) each(output(count())))"); + } + + @Test + void raw_attribute_cannot_be_used_for_lookup() { + try { + var builder = new AttributesConfig.Builder(); + builder.attribute(new AttributesConfig.Attribute.Builder().name("map.key") + .datatype(AttributesConfig.Attribute.Datatype.RAW)); + builder.attribute(new AttributesConfig.Attribute.Builder().name("map.value") + .datatype(AttributesConfig.Attribute.Datatype.STRING)); + builder.attribute(new AttributesConfig.Attribute.Builder().name("key_source") + .datatype(AttributesConfig.Attribute.Datatype.RAW)); + validateGrouping(new AttributesConfig(builder), + "all(group(map{attribute(key_source)}) each(output(count())))"); + fail("Expected exception"); + } catch (IllegalArgumentException e) { + assertEquals("Grouping request references attribute 'map.key' " + + "with unsupported data type 'RAW' collection type 'SINGLE' for map lookup", + e.getMessage()); + } + } + private static AttributesConfig setupMismatchingKeySourceAttribute(boolean matchingDataType) { AttributesConfig.Builder builder = new AttributesConfig.Builder(); builder.attribute(new AttributesConfig.Attribute.Builder().name("map.key") .datatype(AttributesConfig.Attribute.Datatype.Enum.STRING)); - builder.attribute(new AttributesConfig.Attribute.Builder().name("map.value")); + builder.attribute(new AttributesConfig.Attribute.Builder().name("map.value") + .datatype(AttributesConfig.Attribute.Datatype.STRING)); builder.attribute(new AttributesConfig.Attribute.Builder().name("key_source") .datatype(matchingDataType ? AttributesConfig.Attribute.Datatype.Enum.STRING : AttributesConfig.Attribute.Datatype.Enum.INT32) @@ -172,7 +231,7 @@ private static AttributesConfig createAttributesConfig(Collection attrib AttributesConfig.Builder builder = new AttributesConfig.Builder(); for (String attributeName : attributeNames) { builder.attribute(new AttributesConfig.Attribute.Builder() - .name(attributeName)); + .name(attributeName).datatype(AttributesConfig.Attribute.Datatype.INT32)); } return new AttributesConfig(builder); } From 5d77c9466944ecf88a242950e4527b8435a26c5d Mon Sep 17 00:00:00 2001 From: Tor Egge Date: Tue, 10 Dec 2024 17:36:31 +0100 Subject: [PATCH 2/2] Allow grouping on bool attribute. --- .../search/grouping/GroupingValidator.java | 20 ++++++++++--------- .../grouping/GroupingValidatorTestCase.java | 19 +++++++++++++----- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java b/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java index d2ba4092b1c6..27ff50e28219 100644 --- a/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java +++ b/container-search/src/main/java/com/yahoo/search/grouping/GroupingValidator.java @@ -76,23 +76,25 @@ static private boolean isPrimitiveAttribute(AttributesConfig.Attribute attribute datatype == AttributesConfig.Attribute.Datatype.DOUBLE; } - static private boolean isSingleRawAttribute(AttributesConfig.Attribute attribute) { - return attribute.datatype() == AttributesConfig.Attribute.Datatype.RAW && + static private boolean isSingleRawOrBoolAttribute(AttributesConfig.Attribute attribute) { + var datatype = attribute.datatype(); + return (datatype == AttributesConfig.Attribute.Datatype.RAW || + datatype == AttributesConfig.Attribute.Datatype.BOOL) && attribute.collectiontype() == AttributesConfig.Attribute.Collectiontype.SINGLE; } - private void verifyHasAttribute(String attributeName, boolean allowSingleRawAttribute) { + private void verifyHasAttribute(String attributeName, boolean isMapLookup) { var attribute = attributes.get(attributeName); if (attribute == null) { throw new UnavailableAttributeException(clusterName, attributeName); } - if (isPrimitiveAttribute(attribute) || (isSingleRawAttribute(attribute) && allowSingleRawAttribute)) { + if (isPrimitiveAttribute(attribute) || (!isMapLookup && isSingleRawOrBoolAttribute(attribute))) { return; } throw new IllegalInputException("Grouping request references attribute '" + attributeName + "' with unsupported data type '" + attribute.datatype() + "' collection type '" + attribute.collectiontype() + "'" + - (allowSingleRawAttribute ? "" : " for map lookup")); + (isMapLookup ? " for map lookup" : "")); } private void verifyCompatibleAttributeTypes(String keyAttributeName, @@ -116,14 +118,14 @@ private class MyVisitor implements ExpressionVisitor { @Override public void visitExpression(GroupingExpression exp) { if (exp instanceof AttributeMapLookupValue mapLookup) { - verifyHasAttribute(mapLookup.getKeyAttribute(), false); - verifyHasAttribute(mapLookup.getValueAttribute(), false); + verifyHasAttribute(mapLookup.getKeyAttribute(), true); + verifyHasAttribute(mapLookup.getValueAttribute(), true); if (mapLookup.hasKeySourceAttribute()) { - verifyHasAttribute(mapLookup.getKeySourceAttribute(), false); + verifyHasAttribute(mapLookup.getKeySourceAttribute(), true); verifyCompatibleAttributeTypes(mapLookup.getKeyAttribute(), mapLookup.getKeySourceAttribute()); } } else if (exp instanceof AttributeValue) { - verifyHasAttribute(((AttributeValue) exp).getAttributeName(), true); + verifyHasAttribute(((AttributeValue) exp).getAttributeName(), false); } } } diff --git a/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java b/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java index 98a147a0fd4c..188db8332b05 100644 --- a/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/grouping/GroupingValidatorTestCase.java @@ -159,6 +159,14 @@ private static void unsupported_attribute_type_throws(String name, AttributesCon } } + private static void supported_attribute_type_works(String name, AttributesConfig.Attribute.Datatype.Enum datatype) { + var builder = new AttributesConfig.Builder(); + builder.attribute(new AttributesConfig.Attribute.Builder().name(name).datatype(datatype)); + validateGrouping(new AttributesConfig(builder), + "all(group(" + name + ") each(output(count())))"); + + } + @Test void tensor_attribute_throws() { unsupported_attribute_type_throws("tensor", AttributesConfig.Attribute.Datatype.TENSOR); @@ -176,11 +184,12 @@ void reference_attribute_throws() { @Test void raw_attribute_is_ok() { - var builder = new AttributesConfig.Builder(); - builder.attribute(new AttributesConfig.Attribute.Builder().name("raw") - .datatype(AttributesConfig.Attribute.Datatype.RAW)); - validateGrouping(new AttributesConfig(builder), - "all(group(raw) each(output(count())))"); + supported_attribute_type_works("raw", AttributesConfig.Attribute.Datatype.RAW); + } + + @Test + void bool_attribute_is_ok() { + supported_attribute_type_works("mybool", AttributesConfig.Attribute.Datatype.BOOL); } @Test