Skip to content
This repository has been archived by the owner on Jun 26, 2024. It is now read-only.

fix(explore): return early, if no entities #196

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

skjindal93
Copy link
Contributor

No description provided.

@skjindal93 skjindal93 requested a review from a team as a code owner March 7, 2024 23:44
Copy link

github-actions bot commented Mar 7, 2024

Test Results

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

Results for commit 31f2447. ± Comparison against base commit f284959.

This pull request removes 32 and adds 17 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"

org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [10] simple-selection
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [11] grouped-time-aggregation-with-group-limit-and-the-rest
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [12] time-aggregations-space-filtered
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [13] group-by-with-group-limit
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [14] aggregations-with-groupby-and-the-rest-deprecated
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [15] grouped-time-aggregation-with-group-limit
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [1] grouped-time-aggregation-with-group-limit-and-the-rest-with-null-value-group
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [2] grouped-time-aggregation-with-group-limit-with-null-and-nonnull-value-group
…

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

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

Project coverage is 81.30%. Comparing base (f284959) to head (31f2447).

Files Patch % Lines
...rtrace/gateway/service/explore/RequestHandler.java 42.85% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #196      +/-   ##
============================================
- Coverage     81.35%   81.30%   -0.05%     
  Complexity     1371     1371              
============================================
  Files           126      126              
  Lines          6200     6205       +5     
  Branches        524      526       +2     
============================================
+ Hits           5044     5045       +1     
- Misses          897      899       +2     
- Partials        259      261       +2     
Flag Coverage Δ
unit 81.30% <50.00%> (-0.05%) ⬇️

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.

aaron-steinfeld
aaron-steinfeld previously approved these changes Mar 8, 2024
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments

@@ -113,12 +118,18 @@ QueryRequest buildQueryRequest(
attributeMetadataProvider.getAttributesMetadata(requestContext, request.getContext());
if (hasOnlyAttributeSource(request.getFilter(), AttributeSource.EDS, attributeMetadataMap)) {
entityIds = getEntityIdsToFilterFromSourceEDS(requestContext, request, attributeMetadataMap);

if (entityIds.isEmpty()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - worth a comment explaining why we shouldn't build a query request in this case

return ExploreResponse.newBuilder();
}

QueryRequest queryRequest = maybeQueryRequest.get();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - prefer .orElseThrow over .get (or move code around to avoid the direct access)

@aaron-steinfeld aaron-steinfeld dismissed their stale review March 8, 2024 00:17

hang on - checking response format

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants