Removed unused array

[dk2k]

Removed unused array

[github-actions]

✅ Gradle check result for fd3bb33: SUCCESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.00%. Comparing base (ec7b652) to head (d12be33).
Report is 102 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #15364   +/-   ##
=========================================
  Coverage     72.00%   72.00%           
+ Complexity    64817    64783   -34     
=========================================
  Files          5307     5307           
  Lines        302660   302657    -3     
  Branches      43724    43723    -1     
=========================================
+ Hits         217931   217942   +11     
+ Misses        66906    66825   -81     
- Partials      17823    17890   +67     

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

[dblock]

I wonder if this is hiding a bug and the intermediate value was supposed to be used. Maybe someone who understands agg can take a look? @Bukhtawar?

[sandeshkr419]

I looked up the commit which added it: 2f38aeb#diff-2d2b27a2755fd2a1d43780adc7cda194f64673c4bc655c5a80c1c167cb5bd1e9 No other references of slice anywhere other than what is removed in this PR.

Also, tried finding its reference in code, looks like this got added as a dead code in the above commit.

Was this intended to be used below? Is it slow to do aggregations[index][ord]?

@dblock I don't think slowing things down would have been the intention.

@msfroh Is there an uncompleted implementation of some optimization that we may be overlooking?

[dblock]

@dblock I don't think slowing things down would have been the intention.

I meant that below the code does aggregations[index][ord]. This could have been the beginning of an optimization for that call.

[github-actions]

✅ Gradle check result for d12be33: SUCCESS

[dblock]

I'm leaving this to @msfroh who understands what this code does to decide to merge/not. Thanks for your help @dk2k!

[msfroh]

This looks correct to me.

It feels very much like this was left over from some refactoring within the commit that introduced this array.

Specifically, I'm pretty sure that at one point line 229 (now line 225) was:

result[ord] = InternalAggregations.from(Arrays.asList(slice));

I suspect that either the original author or their reviewer objected to the number of additional objects/arrays being created, so they suggested wrapping list views over existing 2D arrays (but in column order, rather than row order).

Of course, they still left the code that created the unnecessary array -- they just let it get garbage-collected right away.

[dblock]

@msfroh So ok to merge as is? Hit merge?

[opensearch-trigger-bot]

This PR is stalled because it has been open for 30 days with no activity.

[sandeshkr419]

@dblock @msfroh Let's hit merge & close on this one?