perf(entities): optimise AND filter query #193

skjindal93 · 2024-02-21T08:03:11Z

No description provided.

github-actions · 2024-02-21T08:04:18Z

Test Results

366 tests ±0 366 ✅ ±0 9s ⏱️ -1s
67 suites ±0 0 💤 ±0
67 files ±0 0 ❌ ±0

Results for commit 271eab2. ± Comparison against base commit fe6c625.

This pull request removes 17 and adds 2 tests. Note that renamed tests count towards both.


      long: 60
      string: "PT1M"
      valueType: LONG
      valueType: STRING
    alias: "numCalls"
    columnName: "SERVICE.numCalls"
    value {
    }
  columnIdentifier {
…

org.hypertrace.gateway.service.baseline.BaselineServiceImplTest ‑ [1] function: AVGRATE
arguments {
  columnIdentifier {
    columnName: "SERVICE.numCalls"
    alias: "numCalls"
  }
}
arguments {
  literal {
    value {
      valueType: STRING
      string: "PT1M"
    }
  }
}
alias: "numCalls"

org.hypertrace.gateway.service.baseline.BaselineServiceImplTest ‑ [2] function: AVGRATE
arguments {
  columnIdentifier {
    columnName: "SERVICE.numCalls"
    alias: "numCalls"
  }
}
arguments {
  literal {
    value {
      valueType: LONG
      long: 60
    }
  }
}
alias: "numCalls"

♻️ This comment has been updated with latest results.

skjindal93 · 2024-02-21T08:03:54Z

gateway-service-impl/src/main/java/org/hypertrace/gateway/service/common/ExpressionContext.java

@@ -18,10 +18,12 @@
 import java.util.stream.Stream;


logic class

skjindal93 · 2024-02-21T08:04:54Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

@@ -1,12 +1,19 @@
 package org.hypertrace.gateway.service.entity.query;


logic class

skjindal93 · 2024-02-21T08:05:04Z

...-service-impl/src/main/java/org/hypertrace/gateway/service/entity/query/DataFetcherNode.java

@@ -19,13 +19,26 @@ public class DataFetcherNode implements QueryNode {
  private Integer limit;


logic class

skjindal93 · 2024-02-21T08:05:30Z

...impl/src/main/java/org/hypertrace/gateway/service/entity/query/visitor/ExecutionVisitor.java

@@ -30,6 +30,7 @@
 import org.hypertrace.gateway.service.entity.query.NoOpNode;


logic class

skjindal93 · 2024-02-21T08:09:47Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

@@ -305,6 +316,115 @@ QueryNode buildFilterTree(EntitiesRequest entitiesRequest, Filter filter) {
    }
  }

+  QueryNode buildAndFilterTree(EntitiesRequest entitiesRequest) {


The most important logic

skjindal93 · 2024-02-21T08:09:58Z

...impl/src/main/java/org/hypertrace/gateway/service/entity/query/visitor/ExecutionVisitor.java

@@ -118,6 +119,19 @@ protected static EntityResponse union(List<EntityResponse> entityResponses) {

  @Override
  public EntityResponse visit(DataFetcherNode dataFetcherNode) {
+    QueryNode childNode = dataFetcherNode.getChildNode();


The most important logic

avinashkolluru · 2024-02-22T16:03:45Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

-    QueryNode optimizedFilterTree = filterTree.acceptVisitor(new FilterOptimizingVisitor());
-    if (LOG.isDebugEnabled()) {
-      LOG.debug("Optimized Filter Tree:{}", optimizedFilterTree.acceptVisitor(new PrintVisitor()));
+    boolean isAndFilter = executionContext.getExpressionContext().isAndFilter();


Didnt understand this. The comment says optimization visitor is not needed for AND filter but the condition is checking for isAndFilter. I'm confused.

Thanks for catching it. I have fixed it. It should have been !isAndFilter

kotharironak · 2024-02-27T11:22:22Z

...-service-impl/src/main/java/org/hypertrace/gateway/service/entity/query/DataFetcherNode.java

+  public DataFetcherNode(String source, Filter filter, QueryNode childNode) {
+    this.source = source;
+    this.filter = filter;
+    this.childNode = childNode;


Is there any restriction or limitation on the type of data a childNode fetch node can have?

kotharironak · 2024-02-27T11:24:32Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

@@ -147,13 +155,19 @@ public QueryNode build() {
     * {@link FilterOptimizingVisitor} is needed to merge filters corresponding to the same source
     * into one {@link DataFetcherNode}, instead of having multiple {@link DataFetcherNode}s for
     * each filter
+     *
+     * <p>It is not needed for AND filter, since the filter tree is already optimised with a single


Is the same true for OR filter too?

kotharironak · 2024-02-27T11:28:04Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

@@ -305,6 +316,104 @@ QueryNode buildFilterTree(EntitiesRequest entitiesRequest, Filter filter) {
    }
  }

+  QueryNode buildAndFilterTree(EntitiesRequest entitiesRequest) {
+    // If the filter by and order by are from QS, pagination can be pushed down to QS
+    // Since the filter and order by are from QS, there won't be any filter on other


Isn't the single source fetch is already handled at the beginning of the function?

kotharironak · 2024-02-27T11:31:59Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

+        AttributeSource source = entry.getKey();
+        Filter andFilter = entry.getValue();
+
+        dataFetcherNodes.add(new DataFetcherNode(source.name(), andFilter, qsNode));


This make sure that first we get the data from QS as its child node, correct?

kotharironak · 2024-02-27T11:34:02Z

...impl/src/main/java/org/hypertrace/gateway/service/entity/query/visitor/ExecutionVisitor.java

+      childFilter = Filter.getDefaultInstance();
+    } else {
+      // Construct the filter from the child nodes result
+      childEntityFetcherResponse = childNodeResponse.getEntityFetcherResponse();


Are we restricting the max number of entities in the sub-sequent filter?

avinashkolluru · 2024-02-28T08:19:11Z

@skjindal93 other change look fine to me. But, the build has been failing. Please check and fix.

codecov · 2024-02-28T11:19:21Z

Codecov Report

Attention: Patch coverage is 43.18182% with 100 lines in your changes are missing coverage. Please review.

Project coverage is 81.35%. Comparing base (fe6c625) to head (271eab2).

Files	Patch %	Lines
...way/service/entity/query/ExecutionTreeBuilder.java	15.49%	57 Missing and 3 partials ⚠️
.../gateway/service/entity/query/DataFetcherNode.java	35.00%	12 Missing and 1 partial ⚠️
...service/entity/query/visitor/ExecutionVisitor.java	48.00%	9 Missing and 4 partials ⚠️
...race/gateway/service/common/ExpressionContext.java	20.00%	7 Missing and 1 partial ⚠️
...ay/service/common/config/GatewayServiceConfig.java	69.23%	3 Missing and 1 partial ⚠️
...pertrace/gateway/service/entity/EntityService.java	77.77%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #193      +/-   ##
============================================
- Coverage     82.56%   81.35%   -1.21%     
- Complexity     1367     1371       +4     
============================================
  Files           125      126       +1     
  Lines          6068     6200     +132     
  Branches        501      524      +23     
============================================
+ Hits           5010     5044      +34     
- Misses          807      897      +90     
- Partials        251      259       +8

Flag	Coverage Δ
unit	`81.35% <43.18%> (-1.21%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kotharironak

Overall looks fine.

kotharironak · 2024-02-28T13:05:36Z

...ce-impl/src/main/java/org/hypertrace/gateway/service/common/config/GatewayServiceConfig.java

+import org.hypertrace.gateway.service.entity.config.LogConfig;
+
+public class GatewayServiceConfig {
+  private static final String ENTITY_AND_FILTER_ENABLED_CONFIG_KEY = "filter.entity.and.enabled";


entity.andFilter.enabled looks more readable.

kotharironak · 2024-02-28T13:10:04Z

...-service-impl/src/main/java/org/hypertrace/gateway/service/entity/query/DataFetcherNode.java

+    this.canFetchTotal = false;
+  }
+
+  public DataFetcherNode(String source, Filter filter, QueryNode childNode) {


As mentioned prior, Should we be restrictive that DataFetcherNode can have only another DataFetcherNode as child or NoOp node?

kotharironak · 2024-02-28T13:17:42Z

...ice-impl/src/main/java/org/hypertrace/gateway/service/entity/query/ExecutionTreeBuilder.java

+    Map<AttributeSource, Filter> sourceToAndFilterMap =
+        new EnumMap<>(buildSourceToAndFilterMap(entitiesRequest.getFilter()));
+
+    // qs node as the pivot node to fetch time range data


So, there won't be any query w/o time range filter, right?

kotharironak · 2024-02-28T13:22:23Z

...impl/src/main/java/org/hypertrace/gateway/service/entity/query/visitor/ExecutionVisitor.java

@@ -128,6 +140,15 @@ public EntityResponse visit(DataFetcherNode dataFetcherNode) {
            entitiesRequest.getEntityType(),
            executionContext.getTimestampAttributeId());

+    Filter entitiesRequestFilter =
+        childFilter != null


When result is empty - it seems that the method constructFilterFromChildNodesResult returns Filter.getDefaultInstance()
Is the defaultInstance equals null?

Is the defaultInstance equals null?

No. It's just an empty filter instance

kotharironak · 2024-02-28T13:26:54Z

...impl/src/main/java/org/hypertrace/gateway/service/entity/query/visitor/ExecutionVisitor.java

+      EntityFetcherResponse entityFetcherResponse = entityFetcher.getEntities(context, request);
+      response =
+          childEntityFetcherResponse != null
+              ? intersectEntities(List.of(childEntityFetcherResponse, entityFetcherResponse))


Curious Q: Do we still need to intersect when childEntityFetcherResponse is not null? In that case, wouldn't the child filter have already applied in subsequent query?

intersectEntities also takes care of merging the selection attributes fetched from various sources

If we fetch attribute A from QS, and attribute B from EDS for the same entity, intersectEntities would merge the responses, and have both A and B added as an attribute on the entity response

kotharironak · 2024-02-28T13:32:50Z

gateway-service/src/main/resources/configs/common/application.conf

@@ -69,6 +69,10 @@ entity.idcolumn.config = [
  },
 ]

+filter.entity = {


entity.andFilter.enabled?

I usually follow a top-down hierarchial approach while building configs, so that the configs are re-usable at a parent level

In this case, filter being the parent can be extended to other configs, and so on

While reading the code, all variable names are isEntityAndFilterEnabled, but the corresponding configuration name differs in the application.conf file. This makes it a bit difficult to easily co-relate.

perf(entities): optimise AND filter query

a700ec7

remove time tracking logs

151d11b

skjindal93 commented Feb 21, 2024

View reviewed changes

avinashkolluru reviewed Feb 22, 2024

View reviewed changes

fix and filter for filter optimizing visitor

bfa41b5

skjindal93 marked this pull request as ready for review February 26, 2024 07:04

skjindal93 requested a review from a team as a code owner February 26, 2024 07:04

skjindal93 requested a review from avinashkolluru February 26, 2024 07:04

kotharironak reviewed Feb 27, 2024

View reviewed changes

fix tests

271eab2

skjindal93 requested a review from kotharironak February 28, 2024 11:16

kotharironak reviewed Feb 28, 2024

View reviewed changes

kotharironak approved these changes Feb 28, 2024

View reviewed changes

skjindal93 merged commit d05ac93 into main Feb 28, 2024
7 of 9 checks passed

skjindal93 deleted the optimise+entities+query+and+filter branch February 28, 2024 13:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(entities): optimise AND filter query #193

perf(entities): optimise AND filter query #193

skjindal93 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024 •

edited

Loading

skjindal93 Feb 21, 2024

skjindal93 Feb 21, 2024

skjindal93 Feb 21, 2024

skjindal93 Feb 21, 2024

skjindal93 Feb 21, 2024

skjindal93 Feb 21, 2024

avinashkolluru Feb 22, 2024

skjindal93 Feb 26, 2024

kotharironak Feb 27, 2024

kotharironak Feb 27, 2024

kotharironak Feb 27, 2024

kotharironak Feb 27, 2024

kotharironak Feb 27, 2024

avinashkolluru commented Feb 28, 2024

codecov bot commented Feb 28, 2024

kotharironak left a comment

kotharironak Feb 28, 2024

kotharironak Feb 28, 2024

kotharironak Feb 28, 2024

skjindal93 Feb 28, 2024

kotharironak Feb 28, 2024

skjindal93 Feb 28, 2024

kotharironak Feb 28, 2024

skjindal93 Feb 28, 2024

kotharironak Feb 28, 2024

skjindal93 Feb 28, 2024

kotharironak Feb 28, 2024

		@@ -1,12 +1,19 @@
		package org.hypertrace.gateway.service.entity.query;

		@@ -19,13 +19,26 @@ public class DataFetcherNode implements QueryNode {
		private Integer limit;

		@@ -30,6 +30,7 @@
		import org.hypertrace.gateway.service.entity.query.NoOpNode;

perf(entities): optimise AND filter query #193

perf(entities): optimise AND filter query #193

Conversation

skjindal93 commented Feb 21, 2024

github-actions bot commented Feb 21, 2024 • edited Loading

Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avinashkolluru commented Feb 28, 2024

codecov bot commented Feb 28, 2024

Codecov Report

kotharironak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 21, 2024 •

edited

Loading