fix(explore): order by on eds attributes

[skjindal93]

Description

Order by on EDS attributes in explore, had missing order by expressions set on the explore request context, which are needed later to group by, and order in memory later

Testing

Verified the changes end to end

[codecov]

Codecov Report

Merging #127 (9680686) into main (1997536) will decrease coverage by 0.25%.
The diff coverage is 9.09%.

❗ Current head 9680686 differs from pull request most recent head 53df6ea. Consider uploading reports for the commit 53df6ea to get more accurate results

@@             Coverage Diff              @@
##               main     #127      +/-   ##
============================================
- Coverage     81.40%   81.16%   -0.25%     
- Complexity     1327     1328       +1     
============================================
  Files           125      125              
  Lines          5964     5982      +18     
  Branches        490      491       +1     
============================================
  Hits           4855     4855              
- Misses          865      884      +19     
+ Partials        244      243       -1     

Flag	Coverage Δ
unit	81.16% <9.09%> (-0.25%)	⬇️

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

Files	Coverage Δ
...rtrace/gateway/service/explore/ExploreService.java	82.85% <100.00%> (+0.91%)	⬆️
...y/service/explore/entity/EntityRequestHandler.java	84.95% <ø> (ø)
...ice/explore/entity/EntityServiceEntityFetcher.java	7.31% <4.76%> (-0.75%)	⬇️

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

[github-actions]

Unit Test Results

  60 files  ±0    60 suites  ±0   8s ⏱️ -1s
337 tests ±0  337 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit f9483a2. ± Comparison against base commit 9d710f8.

[github-actions]

Test Results

3 tests   - 358   3 ✔️  - 358   0s ⏱️ -10s
1 suites  -   65   0 💤 ±    0 
1 files    -   65   0 ❌ ±    0 

Results for commit 53df6ea. ± Comparison against base commit 1997536.

This pull request removes 373 tests.

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

♻️ This comment has been updated with latest results.

[aaron-steinfeld]

I'm a bit confused by this comment - aren't we pushing down the order here? Why can't we push down limit/offset?

[skjindal93]

We aren't pushing order by to EDS in case of explore. EDS handler is only called when

• there is a group by, order by and selections on EDS. In this case, we handle it in memory. We can push group by to EDS along with order by, which is being called out by the TODO comment. That requires a separate effort
• there is an aggregated selection on EDS (like DISTINCT COUNT of APIs). There is no order by here. And, the limit in this case would be 1 inherently

[aaron-steinfeld]

Second case makes sense.

First case - I thought we were pushing down group by (line 61). Discussed offline, but since this is the final query and isn't being joined with anything in memory (right?) we should be able to push everything down unless there's something missing support in EQS.

[skjindal93]

Tried out different queries. We don't support ordering in document store on function expressions. Since, that's the case, we can't push limit/offset and order bys to EQS

We are already pushing group bys to EQS. Pushing limit and offset to EQS, only if there are no order by expressions (which shouldn't ideally be the case, since group by mostly comes with order by)

orderBy: [
  {
    aggregation: COUNT
    size: null
    direction: DESC
    keyExpression: { key: "name" }
  }
]

[aaron-steinfeld]

Second case makes sense.

First case - I thought we were pushing down group by (line 61). Discussed offline, but since this is the final query and isn't being joined with anything in memory (right?) we should be able to push everything down unless there's something missing support in EQS.

[aaron-steinfeld]

Can we move this code into addLimitAndOffset? It's confusing that we set the limit and offset unconditionally (in addLimitAndOffset), then conditionally overwrite them in the top level buildRequest call.

Also whether we push down or not will influence response processing. We should make sure we're in sync (i.e. have common code or state so we only evaluate the condition in one place in code).