fix | fix the entity ids filter for explore if empty (#197)

* fix | fix the entity ids filter for explore if empty

Author: aman-bansal

gateway-service-impl/src/main/java/org/hypertrace/gateway/service/explore/RequestHandler.java

@@ -5,7 +5,7 @@
 import com.google.common.collect.Streams;
 import com.google.protobuf.InvalidProtocolBufferException;
 import com.google.protobuf.util.JsonFormat;
-import java.util.ArrayList;
+import io.grpc.Status;
 import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
@@ -35,10 +35,14 @@
 import org.hypertrace.gateway.service.entity.EntitiesRequestContext;
 import org.hypertrace.gateway.service.entity.config.EntityIdColumnsConfig;
 import org.hypertrace.gateway.service.explore.entity.EntityServiceEntityFetcher;
+import org.hypertrace.gateway.service.v1.common.AttributeExpression;
 import org.hypertrace.gateway.service.v1.common.Expression;
 import org.hypertrace.gateway.service.v1.common.FunctionExpression;
+import org.hypertrace.gateway.service.v1.common.LiteralConstant;
+import org.hypertrace.gateway.service.v1.common.Operator;
 import org.hypertrace.gateway.service.v1.common.OrderByExpression;
 import org.hypertrace.gateway.service.v1.common.TimeAggregation;
+import org.hypertrace.gateway.service.v1.common.ValueType;
 import org.hypertrace.gateway.service.v1.entity.EntitiesRequest;
 import org.hypertrace.gateway.service.v1.entity.Entity.Builder;
 import org.hypertrace.gateway.service.v1.explore.ExploreRequest;
@@ -63,6 +67,17 @@
  */
 public class RequestHandler implements RequestHandlerWithSorting {
   private static final Logger LOG = LoggerFactory.getLogger(RequestHandler.class);
+  private static final Expression NULL_VALUE_EXPRESSION =
+      Expression.newBuilder()
+          .setLiteral(
+              LiteralConstant.newBuilder()
+                  .setValue(
+                      org.hypertrace.gateway.service.v1.common.Value.newBuilder()
+                          .setValueType(ValueType.STRING)
+                          .setString("null")
+                          .build())
+                  .build())
+          .build();
 
   private final QueryServiceClient queryServiceClient;
   private final AttributeMetadataProvider attributeMetadataProvider;
@@ -107,15 +122,30 @@ QueryRequest buildQueryRequest(
       requestContext.setHasGroupBy(true);
     }
 
-    List<String> entityIds = new ArrayList<>();
     org.hypertrace.gateway.service.v1.common.Filter qsSourceFilter = request.getFilter();
     Map<String, AttributeMetadata> attributeMetadataMap =
         attributeMetadataProvider.getAttributesMetadata(requestContext, request.getContext());
     if (hasOnlyAttributeSource(request.getFilter(), AttributeSource.EDS, attributeMetadataMap)) {
-      entityIds = getEntityIdsToFilterFromSourceEDS(requestContext, request, attributeMetadataMap);
+      List<String> entityIds =
+          getEntityIdsToFilterFromSourceEDS(requestContext, request, attributeMetadataMap);
+      List<String> entityIdAttributes =
+          AttributeMetadataUtil.getIdAttributeIds(
+              attributeMetadataProvider,
+              entityIdColumnsConfig,
+              requestContext,
+              request.getContext());
       qsSourceFilter =
-          buildFilter(request.getFilter(), AttributeSource.QS, attributeMetadataMap)
-              .orElse(request.getFilter());
+          org.hypertrace.gateway.service.v1.common.Filter.newBuilder()
+              .setOperator(Operator.AND)
+              .addChildFilter(
+                  buildFilter(request.getFilter(), AttributeSource.QS, attributeMetadataMap)
+                      .orElse(request.getFilter()))
+              .addChildFilter(
+                  createEntityIdAttributeFilter(
+                      entityIdAttributes,
+                      org.hypertrace.gateway.service.v1.common.Operator.IN,
+                      entityIds))
+              .build();
     }
 
     QueryRequest.Builder builder = QueryRequest.newBuilder();
@@ -141,7 +171,7 @@ QueryRequest buildQueryRequest(
     // 2. Add filter
     builder.setFilter(
         constructQueryServiceFilter(
-            request, qsSourceFilter, requestContext, attributeMetadataProvider, entityIds));
+            request, qsSourceFilter, requestContext, attributeMetadataProvider));
 
     if (requestContext.hasGroupBy() && request.getIncludeRestGroup() && request.getOffset() > 0) {
       // including rest group with offset is an invalid combination
@@ -280,33 +310,22 @@ Filter constructQueryServiceFilter(
       ExploreRequestContext exploreRequestContext,
       AttributeMetadataProvider attributeMetadataProvider) {
     return this.constructQueryServiceFilter(
-        request,
-        request.getFilter(),
-        exploreRequestContext,
-        attributeMetadataProvider,
-        Collections.emptyList());
+        request, request.getFilter(), exploreRequestContext, attributeMetadataProvider);
   }
 
   Filter constructQueryServiceFilter(
       ExploreRequest request,
       org.hypertrace.gateway.service.v1.common.Filter requestFilter,
       ExploreRequestContext exploreRequestContext,
-      AttributeMetadataProvider attributeMetadataProvider,
-      List<String> entityIds) {
-    return QueryAndGatewayDtoConverter.addTimeSpaceAndIdFiltersAndConvertToQueryFilter(
+      AttributeMetadataProvider attributeMetadataProvider) {
+    return QueryAndGatewayDtoConverter.addTimeAndSpaceFiltersAndConvertToQueryFilter(
         request.getStartTimeMillis(),
         request.getEndTimeMillis(),
         request.getSpaceId(),
-        entityIds,
         AttributeMetadataUtil.getTimestampAttributeId(
             attributeMetadataProvider, exploreRequestContext, request.getContext()),
         AttributeMetadataUtil.getSpaceAttributeId(
             attributeMetadataProvider, exploreRequestContext, request.getContext()),
-        AttributeMetadataUtil.getIdAttributeIds(
-            attributeMetadataProvider,
-            entityIdColumnsConfig,
-            exploreRequestContext,
-            request.getContext()),
         requestFilter);
   }
 
@@ -532,6 +551,72 @@ private Map<String, AttributeMetadata> remapAttributeMetadataByResultName(
         attributeMetadataByIdMap);
   }
 
+  private org.hypertrace.gateway.service.v1.common.Filter createEntityIdAttributeFilter(
+      List<String> entityIdAttributes,
+      org.hypertrace.gateway.service.v1.common.Operator operator,
+      List<String> entityIds) {
+    if (entityIdAttributes.size() != 1) {
+      throw Status.FAILED_PRECONDITION
+          .withDescription("entity must have one id attribute.")
+          .asRuntimeException();
+    }
+
+    if (entityIds.isEmpty()) {
+      // TODO: have a better approach here. One possible solution could be to
+      //  form a dummy response based on the selections provided
+
+      // Having empty entity ids is valid filter because this means that
+      // EDS source has filtered out all the entities and the result should
+      // be empty. But QS doesn't recognize empty IN filter as valid filter
+      // and ignoring this filter will generate wrong results.
+      //
+      // We could have return empty but clients expects response to contain
+      // information as per the selections they have sent. So we are converting
+      // empty entity Id filter into a false filter (id != null && id == null)
+      // so that results are always empty but must contain the valid selections.
+      return org.hypertrace.gateway.service.v1.common.Filter.newBuilder()
+          .setOperator(Operator.AND)
+          .addChildFilter(
+              org.hypertrace.gateway.service.v1.common.Filter.newBuilder()
+                  .setLhs(buildAttributeExpression(entityIdAttributes.get(0)))
+                  .setOperator(Operator.EQ)
+                  .setRhs(NULL_VALUE_EXPRESSION)
+                  .build())
+          .addChildFilter(
+              org.hypertrace.gateway.service.v1.common.Filter.newBuilder()
+                  .setLhs(buildAttributeExpression(entityIdAttributes.get(0)))
+                  .setOperator(Operator.NEQ)
+                  .setRhs(NULL_VALUE_EXPRESSION)
+                  .build())
+          .build();
+    }
+
+    return org.hypertrace.gateway.service.v1.common.Filter.newBuilder()
+        .setLhs(buildAttributeExpression(entityIdAttributes.get(0)))
+        .setOperator(operator)
+        .setRhs(buildStringArrayExpression(entityIds))
+        .build();
+  }
+
+  private Expression buildStringArrayExpression(List<String> values) {
+    return Expression.newBuilder()
+        .setLiteral(
+            LiteralConstant.newBuilder()
+                .setValue(
+                    org.hypertrace.gateway.service.v1.common.Value.newBuilder()
+                        .setValueType(ValueType.STRING_ARRAY)
+                        .addAllStringArray(values)
+                        .build())
+                .build())
+        .build();
+  }
+
+  private Expression buildAttributeExpression(String value) {
+    return Expression.newBuilder()
+        .setAttributeExpression(AttributeExpression.newBuilder().setAttributeId(value).build())
+        .build();
+  }
+
   private boolean hasOnlyAttributeSource(
       org.hypertrace.gateway.service.v1.common.Filter filter,
       AttributeSource source,

gateway-service-impl/src/test/java/org/hypertrace/gateway/service/explore/RequestHandlerTest.java

@@ -639,36 +639,6 @@ private QueryRequest getQueryRequest() throws InvalidProtocolBufferException {
                 + "  \"filter\": {\n"
                 + "    \"childFilter\": [{\n"
                 + "      \"childFilter\": [{\n"
-                + "        \"lhs\": {\n"
-                + "          \"attributeExpression\": {\n"
-                + "            \"attributeId\": \"API.timestampId\"\n"
-                + "          }\n"
-                + "        },\n"
-                + "        \"operator\": \"GE\",\n"
-                + "        \"rhs\": {\n"
-                + "          \"literal\": {\n"
-                + "            \"value\": {\n"
-                + "              \"valueType\": \"LONG\"\n"
-                + "            }\n"
-                + "          }\n"
-                + "        }\n"
-                + "      }, {\n"
-                + "        \"lhs\": {\n"
-                + "          \"attributeExpression\": {\n"
-                + "            \"attributeId\": \"API.timestampId\"\n"
-                + "          }\n"
-                + "        },\n"
-                + "        \"operator\": \"LT\",\n"
-                + "        \"rhs\": {\n"
-                + "          \"literal\": {\n"
-                + "            \"value\": {\n"
-                + "              \"valueType\": \"LONG\"\n"
-                + "            }\n"
-                + "          }\n"
-                + "        }\n"
-                + "      }]\n"
-                + "    }, {\n"
-                + "      \"childFilter\": [{\n"
                 + "        \"childFilter\": [{\n"
                 + "          \"childFilter\": [{\n"
                 + "            \"lhs\": {\n"