From 8c2c883e3fb96e6159c2dd5a473c1761b1a12104 Mon Sep 17 00:00:00 2001 From: Kshitiz Saxena <84707889+saxenakshitiz@users.noreply.github.com> Date: Tue, 14 Nov 2023 18:05:57 +0530 Subject: [PATCH] chore: handle case of empty child filter (#214) --- .../service/AbstractQueryTransformation.java | 17 +++- .../query/service/AbstractServiceTest.java | 47 ++++++++++ .../query/service/QueryServiceConfigTest.java | 2 +- ...pathExistsFilteringTransformationTest.java | 50 ++++++++++ .../pinot/PinotBasedRequestHandlerTest.java | 91 ++++++++++++++++++- .../src/test/resources/application.conf | 21 ++++- .../query/request-with-empty-filter.json | 84 +++++++++++++++++ 7 files changed, 305 insertions(+), 7 deletions(-) create mode 100644 query-service-impl/src/test/java/org/hypertrace/core/query/service/AbstractServiceTest.java create mode 100644 query-service-impl/src/test/resources/test-requests/query/request-with-empty-filter.json diff --git a/query-service-impl/src/main/java/org/hypertrace/core/query/service/AbstractQueryTransformation.java b/query-service-impl/src/main/java/org/hypertrace/core/query/service/AbstractQueryTransformation.java index fe955ccd..45f3685d 100644 --- a/query-service-impl/src/main/java/org/hypertrace/core/query/service/AbstractQueryTransformation.java +++ b/query-service-impl/src/main/java/org/hypertrace/core/query/service/AbstractQueryTransformation.java @@ -5,6 +5,7 @@ import io.reactivex.rxjava3.core.Observable; import io.reactivex.rxjava3.core.Single; import java.util.List; +import java.util.Optional; import org.hypertrace.core.query.service.api.AttributeExpression; import org.hypertrace.core.query.service.api.ColumnIdentifier; import org.hypertrace.core.query.service.api.Expression; @@ -110,15 +111,23 @@ protected Single transformOrderBy(OrderByExpression orderBy) } protected Single transformFilter(Filter filter) { + return transformFilterInternal(filter) + .map(filterOptional -> filterOptional.orElseGet(Filter::getDefaultInstance)); + } + + private Single> transformFilterInternal(Filter filter) { if (filter.equals(Filter.getDefaultInstance())) { - return Single.just(filter); + return Single.just(Optional.empty()); } Single lhsSingle = this.transformExpression(filter.getLhs()); Single rhsSingle = this.transformExpression(filter.getRhs()); + // remove defaults from child filters Single> childFilterListSingle = Observable.fromIterable(filter.getChildFilterList()) - .concatMapSingle(this::transformFilter) + .concatMapSingle(this::transformFilterInternal) + .filter(Optional::isPresent) + .map(Optional::get) .toList(); return zip( lhsSingle, @@ -143,7 +152,7 @@ private Single> transformOrderByList( * This doesn't change any functional behavior, but omits fields that aren't needed, shrinking the * object and keeping it equivalent to the source object for equality checks. */ - private Filter rebuildFilterOmittingDefaults( + private Optional rebuildFilterOmittingDefaults( Filter original, Expression lhs, Expression rhs, List childFilters) { Filter.Builder builder = original.toBuilder(); @@ -159,7 +168,7 @@ private Filter rebuildFilterOmittingDefaults( builder.setRhs(rhs); } - return builder.clearChildFilter().addAllChildFilter(childFilters).build(); + return Optional.of(builder.clearChildFilter().addAllChildFilter(childFilters).build()); } private void debugLogIfRequestTransformed(QueryRequest original, QueryRequest transformed) { diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/AbstractServiceTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/AbstractServiceTest.java new file mode 100644 index 00000000..504c5ed3 --- /dev/null +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/AbstractServiceTest.java @@ -0,0 +1,47 @@ +package org.hypertrace.core.query.service; + +import com.google.protobuf.GeneratedMessageV3; +import com.google.protobuf.InvalidProtocolBufferException; +import com.google.protobuf.util.JsonFormat; +import java.io.BufferedReader; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.util.stream.Collectors; +import org.hypertrace.core.grpcutils.context.RequestContext; + +public abstract class AbstractServiceTest { + + private static final String QUERY_SERVICE_TEST_REQUESTS_DIR = "test-requests"; + + protected TQueryServiceRequestType readQueryServiceRequest(String fileName) { + String resourceFileName = createResourceFileName(QUERY_SERVICE_TEST_REQUESTS_DIR, fileName); + String requestJsonStr = readResourceFileAsString(resourceFileName); + + GeneratedMessageV3.Builder requestBuilder = getQueryServiceRequestBuilder(); + try { + JsonFormat.parser().merge(requestJsonStr, requestBuilder); + } catch (InvalidProtocolBufferException e) { + e.printStackTrace(); + } + + return (TQueryServiceRequestType) requestBuilder.build(); + } + + private static Reader readResourceFile(String fileName) { + InputStream in = RequestContext.class.getClassLoader().getResourceAsStream(fileName); + return new BufferedReader(new InputStreamReader(in)); + } + + private String readResourceFileAsString(String fileName) { + return ((BufferedReader) readResourceFile(fileName)).lines().collect(Collectors.joining()); + } + + private String createResourceFileName(String filesBaseDir, String fileName) { + return String.format("%s/%s/%s.json", filesBaseDir, getTestSuiteName(), fileName); + } + + protected abstract String getTestSuiteName(); + + protected abstract GeneratedMessageV3.Builder getQueryServiceRequestBuilder(); +} diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java index 485bd485..efc87184 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java @@ -33,7 +33,7 @@ public void testQueryServiceImplConfigParser() { assertEquals("query-service", appConfig.getString("service.name")); assertEquals(8091, appConfig.getInt("service.admin.port")); assertEquals(8090, appConfig.getInt("service.port")); - assertEquals(10, queryServiceConfig.getQueryRequestHandlersConfigs().size()); + assertEquals(11, queryServiceConfig.getQueryRequestHandlersConfigs().size()); assertEquals(3, queryServiceConfig.getRequestHandlerClientConfigs().size()); RequestHandlerConfig handler0 = queryServiceConfig.getQueryRequestHandlersConfigs().get(0); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/attribubteexpression/AttributeExpressionSubpathExistsFilteringTransformationTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/attribubteexpression/AttributeExpressionSubpathExistsFilteringTransformationTest.java index f9a43702..0458dbe1 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/attribubteexpression/AttributeExpressionSubpathExistsFilteringTransformationTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/attribubteexpression/AttributeExpressionSubpathExistsFilteringTransformationTest.java @@ -8,6 +8,7 @@ import static org.hypertrace.core.query.service.QueryRequestUtil.createContainsKeyFilter; import static org.hypertrace.core.query.service.QueryRequestUtil.createStringLiteralValueExpression; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.List; @@ -93,6 +94,55 @@ void transQueryWithComplexAttributeExpression_MultipleFilter() { .contains(createContainsKeyFilter(spanTags2.getAttributeExpression()))); } + @Test + void transQueryWithEmptyFilter() { + Filter emptyFilter = Filter.getDefaultInstance(); + QueryRequest originalRequest = QueryRequest.newBuilder().setFilter(emptyFilter).build(); + + QueryRequest expectedTransform = + this.transformation.transform(originalRequest, mockTransformationContext).blockingGet(); + assertFalse(expectedTransform.hasFilter()); + } + + @Test + void transQueryWithComplexAttributeExpression_EmptyFilter() { + Expression spanTags1 = createComplexAttributeExpression("Span.tags", "FLAGS").build(); + Expression spanTags2 = createComplexAttributeExpression("Span.tags", "span.kind").build(); + + Filter childFilter1 = + Filter.newBuilder() + .setLhs(spanTags1) + .setOperator(Operator.EQ) + .setRhs(createStringLiteralValueExpression("0")) + .build(); + Filter childFilter2 = + Filter.newBuilder() + .setLhs(spanTags2) + .setOperator(Operator.EQ) + .setRhs(createStringLiteralValueExpression("server")) + .build(); + Filter emptyFilter = Filter.getDefaultInstance(); + + Filter.Builder filter = + createCompositeFilter(Operator.AND, childFilter1, childFilter2, emptyFilter); + QueryRequest originalRequest = QueryRequest.newBuilder().setFilter(filter).build(); + + QueryRequest expectedTransform = + this.transformation.transform(originalRequest, mockTransformationContext).blockingGet(); + List childFilterList = expectedTransform.getFilter().getChildFilterList(); + + assertTrue( + childFilterList + .get(0) + .getChildFilterList() + .contains(createContainsKeyFilter(spanTags1.getAttributeExpression()))); + assertTrue( + childFilterList + .get(1) + .getChildFilterList() + .contains(createContainsKeyFilter(spanTags2.getAttributeExpression()))); + } + @Test void transQueryWithComplexAttributeExpression_HierarchicalFilter() { Expression spanTags1 = createComplexAttributeExpression("Span.tags", "FLAGS").build(); diff --git a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java index e31ab9c4..78c38c8c 100644 --- a/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java +++ b/query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java @@ -12,6 +12,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import com.google.protobuf.GeneratedMessageV3; import com.typesafe.config.Config; import com.typesafe.config.ConfigFactory; import io.reactivex.rxjava3.core.Observable; @@ -24,10 +25,13 @@ import java.util.stream.Stream; import org.apache.pinot.client.ResultSet; import org.apache.pinot.client.ResultSetGroup; +import org.hypertrace.core.query.service.AbstractQueryTransformation; +import org.hypertrace.core.query.service.AbstractServiceTest; import org.hypertrace.core.query.service.ExecutionContext; import org.hypertrace.core.query.service.QueryCost; import org.hypertrace.core.query.service.QueryRequestBuilderUtils; import org.hypertrace.core.query.service.QueryRequestUtil; +import org.hypertrace.core.query.service.QueryTransformation.QueryTransformationContext; import org.hypertrace.core.query.service.api.Expression; import org.hypertrace.core.query.service.api.Filter; import org.hypertrace.core.query.service.api.LiteralConstant; @@ -43,8 +47,11 @@ import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; -public class PinotBasedRequestHandlerTest { +public class PinotBasedRequestHandlerTest extends AbstractServiceTest { + private static final Logger LOGGER = LoggerFactory.getLogger(PinotBasedRequestHandlerTest.class); // Test subject private PinotBasedRequestHandler pinotBasedRequestHandler; private final ObjectMapper objectMapper = new ObjectMapper(); @@ -1734,6 +1741,78 @@ public boolean isResultTableResultSetType(ResultSet resultSet) { } } + @Test + public void testEmptyChildFilterCase() throws IOException { + for (Config config : serviceConfig.getConfigList("queryRequestHandlersConfig")) { + if (!isPinotConfig(config)) { + continue; + } + + if (!config.getString("name").equals("domain-service-handler")) { + continue; + } + + // Mock the PinotClient + PinotClient pinotClient = mock(PinotClient.class); + PinotClientFactory factory = mock(PinotClientFactory.class); + when(factory.getPinotClient(any())).thenReturn(pinotClient); + + String[][] resultTable = new String[][] {{"5"}}; + List columnNames = List.of("domain_id"); + ResultSet resultSet = mockResultSet(1, 1, columnNames, resultTable); + ResultSetGroup resultSetGroup = mockResultSetGroup(List.of(resultSet)); + + PinotBasedRequestHandler handler = + new PinotBasedRequestHandler( + config.getString("name"), + config.getConfig("requestHandlerInfo"), + new ResultSetTypePredicateProvider() { + @Override + public boolean isSelectionResultSetType(ResultSet resultSet) { + return true; + } + + @Override + public boolean isResultTableResultSetType(ResultSet resultSet) { + return false; + } + }, + factory); + + QueryRequest request = readQueryServiceRequest("request-with-empty-filter"); + AbstractQueryTransformation transformation = + new AbstractQueryTransformation() { + @Override + protected Logger getLogger() { + return LOGGER; + } + }; + QueryTransformationContext mockTransformationContext = mock(QueryTransformationContext.class); + QueryRequest transformedRequest = + transformation.transform(request, mockTransformationContext).blockingGet(); + ExecutionContext context = new ExecutionContext("__default", transformedRequest); + QueryCost cost = handler.canHandle(transformedRequest, context); + Assertions.assertTrue(cost.getCost() > 0.0d && cost.getCost() < 1d); + + // empty child filter should be ignored + String expectedQuery = + "Select DISTINCTCOUNTHLL(domain_id) FROM domainEventSpanView WHERE customer_id = ?" + + " AND ( domain_id != ? AND ( start_time_millis >= ? AND start_time_millis < ? )" + + " AND ( ( environment IN (?) ) ) ) limit 1"; + Params params = + Params.newBuilder() + .addStringParam("__default") + .addStringParam("null") + .addLongParam(1658818870181L) + .addLongParam(1658905270181L) + .addStringParam("ui-data-validation") + .build(); + when(pinotClient.executeQuery(expectedQuery, params)).thenReturn(resultSetGroup); + + verifyResponseRows(handler.handleRequest(transformedRequest, context), resultTable); + } + } + @ParameterizedTest @MethodSource("provideHandlerValue") public void testGetTimeFilterColumn(int index, String expectedValue) { @@ -1816,4 +1895,14 @@ private String stringify(Object obj) throws JsonProcessingException { private boolean isPinotConfig(Config config) { return config.getString("type").equals("pinot"); } + + @Override + protected String getTestSuiteName() { + return "query"; + } + + @Override + protected GeneratedMessageV3.Builder getQueryServiceRequestBuilder() { + return QueryRequest.newBuilder(); + } } diff --git a/query-service-impl/src/test/resources/application.conf b/query-service-impl/src/test/resources/application.conf index ae3cd467..7953d0cb 100644 --- a/query-service-impl/src/test/resources/application.conf +++ b/query-service-impl/src/test/resources/application.conf @@ -382,5 +382,24 @@ service.config = { } } } + { + name = domain-service-handler + type = pinot + clientConfig = broker + requestHandlerInfo = { + tenantColumnName: customer_id + startTimeAttributeName = "EVENT.startTime" + distinctCountAggFunction = DISTINCTCOUNTHLL + viewDefinition = { + viewName = domainEventSpanView + fieldMap = { + "EVENT.startTime": "start_time_millis", + "DOMAIN.id": "domain_id", + "DOMAIN.name": "domain_name", + "EVENT.environment": "environment" + } + } + } + } ] -} \ No newline at end of file +} diff --git a/query-service-impl/src/test/resources/test-requests/query/request-with-empty-filter.json b/query-service-impl/src/test/resources/test-requests/query/request-with-empty-filter.json new file mode 100644 index 00000000..7d2d8b57 --- /dev/null +++ b/query-service-impl/src/test/resources/test-requests/query/request-with-empty-filter.json @@ -0,0 +1,84 @@ +{ + "filter": { + "childFilter": [{ + "lhs": { + "attributeExpression": { + "attributeId": "DOMAIN.id" + } + }, + "operator": "NEQ", + "rhs": { + "literal": { + "value": { + "valueType": "NULL_STRING" + } + } + } + }, { + "childFilter": [{ + "lhs": { + "attributeExpression": { + "attributeId": "EVENT.startTime", + "alias": "DOMAIN.startTime" + } + }, + "operator": "GE", + "rhs": { + "literal": { + "value": { + "valueType": "LONG", + "long": "1658818870181" + } + } + } + }, { + "lhs": { + "attributeExpression": { + "attributeId": "EVENT.startTime", + "alias": "DOMAIN.startTime" + } + }, + "operator": "LT", + "rhs": { + "literal": { + "value": { + "valueType": "LONG", + "long": "1658905270181" + } + } + } + }] + }, { + "childFilter": [{}, { + "childFilter": [{ + "lhs": { + "attributeExpression": { + "attributeId": "EVENT.environment", + "alias": "environment" + } + }, + "operator": "IN", + "rhs": { + "literal": { + "value": { + "valueType": "STRING_ARRAY", + "stringArray": ["ui-data-validation"] + } + } + } + }] + }] + }] + }, + "selection": [{ + "function": { + "functionName": "DISTINCTCOUNT", + "arguments": [{ + "attributeExpression": { + "attributeId": "DOMAIN.id" + } + }] + } + }], + "limit": 1 +}