chore: handle case of empty child filter (#214)

hypertrace · Nov 14, 2023 · 8c2c883 · 8c2c883
1 parent 436adec
commit 8c2c883
Show file tree

Hide file tree

Showing 7 changed files with 305 additions and 7 deletions.
diff --git a/...ice-impl/src/main/java/org/hypertrace/core/query/service/AbstractQueryTransformation.java b/...ice-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<OrderByExpression> transformOrderBy(OrderByExpression orderBy)
   }
 
   protected Single<Filter> transformFilter(Filter filter) {
+    return transformFilterInternal(filter)
+        .map(filterOptional -> filterOptional.orElseGet(Filter::getDefaultInstance));
+  }
+
+  private Single<Optional<Filter>> transformFilterInternal(Filter filter) {
     if (filter.equals(Filter.getDefaultInstance())) {
-      return Single.just(filter);
+      return Single.just(Optional.empty());
     }
 
     Single<Expression> lhsSingle = this.transformExpression(filter.getLhs());
     Single<Expression> rhsSingle = this.transformExpression(filter.getRhs());
+    // remove defaults from child filters
     Single<List<Filter>> 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<List<OrderByExpression>> 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<Filter> rebuildFilterOmittingDefaults(
       Filter original, Expression lhs, Expression rhs, List<Filter> 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
@@ -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<TQueryServiceRequestType extends GeneratedMessageV3> {
+
+  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/...-service-impl/src/test/java/org/hypertrace/core/query/service/QueryServiceConfigTest.java b/...-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/...ice/attribubteexpression/AttributeExpressionSubpathExistsFilteringTransformationTest.java b/...ice/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<Filter> 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/...l/src/test/java/org/hypertrace/core/query/service/pinot/PinotBasedRequestHandlerTest.java b/...l/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<QueryRequest> {
+  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<String> 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
@@ -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"
+          }
+        }
+      }
+    }
   ]
-}
+}