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

fix(explore): skip child filters for default filter #176

Open
wants to merge 1 commit 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 September 14, 2023 16:56
@github-actions
Copy link

Test Results

361 tests  ±0   361 ✔️ ±0   14s ⏱️ +4s
  66 suites ±0       0 💤 ±0 
  66 files   ±0       0 ±0 

Results for commit 0aa5dd5. ± Comparison against base commit 525fc0d.

This pull request removes 28 and adds 13 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
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [12] simple-aggregations
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [2] time-aggregations-space-filtered
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [3] group-by-with-offset
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [4] aggregations-with-groupby
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [5] group-by-with-group-limit-and-rest
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [6] aggregations-with-groupby-and-the-rest-deprecated
…

@codecov
Copy link

codecov bot commented Sep 14, 2023

Codecov Report

Merging #176 (0aa5dd5) into main (525fc0d) will decrease coverage by 0.05%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main     #176      +/-   ##
============================================
- Coverage     81.43%   81.39%   -0.05%     
  Complexity     1323     1323              
============================================
  Files           125      125              
  Lines          5951     5954       +3     
  Branches        490      491       +1     
============================================
  Hits           4846     4846              
- Misses          861      864       +3     
  Partials        244      244              
Flag Coverage Δ
unit 81.39% <0.00%> (-0.05%) ⬇️

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

Files Changed Coverage Δ
...ice/explore/entity/EntityServiceEntityFetcher.java 7.69% <0.00%> (-0.38%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@@ -93,12 +93,18 @@ private void addSelections(

private Filter.Builder buildFilter(
ExploreRequest exploreRequest, List<String> entityIdAttributeIds, Set<String> entityIds) {
if (org.hypertrace.gateway.service.v1.common.Filter.getDefaultInstance()
.equals(exploreRequest.getFilter())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rely on presence check? or is there an upstream bug? Either way, not sure I'm following why entity IDs are ignored if there's no filter? Shouldn't we just not be and'ing and using only the in filter?

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.

3 participants