Ensure all the lists used in PinotQuery are ArrayList

[Jackie-Jiang]

Refactor the code to ensure the lists used in PinotQuery (thrift object) are all ArrayList.
Thrift internally always use ArrayList, but we might set it with other list types, such as Arrays.asList(), Collections.singletonList() etc. The problem with this is that query optimizer assumes the lists are mutable, and might modify the list. Currently query optimizer only replaces elements, which is supported by Arrays.asList() but not Collections.singletonList(). This PR ensures all lists are ArrayList so that it is also future proof.

We detect this issue by using ordinal in ORDER BY along with expression override. Here is the exception:

Exception: java.lang.UnsupportedOperationException
        at java.base/java.util.Collections$SingletonList.replaceAll(Collections.java:5194)
        at org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleExpressionOverride(BaseBrokerRequestHandler.java:1395)
        at org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:562)
        at org.apache.pinot.broker.requesthandler.BaseBrokerRequestHandler.handleRequest(BaseBrokerRequestHandler.java:290)

[codecov-commenter]

Codecov Report

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

Project coverage is 62.13%. Comparing base (59551e4) to head (5e9bcea).
Report is 381 commits behind head on master.

Files	Patch %	Lines
...roker/requesthandler/BaseBrokerRequestHandler.java	25.00%	6 Missing ⚠️
...org/apache/pinot/sql/parsers/CalciteSqlParser.java	92.85%	1 Missing and 1 partial ⚠️
...ry/runtime/plan/server/ServerPlanRequestUtils.java	84.61%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #13017      +/-   ##
============================================
+ Coverage     61.75%   62.13%   +0.37%     
+ Complexity      207      198       -9     
============================================
  Files          2436     2502      +66     
  Lines        133233   136612    +3379     
  Branches      20636    21162     +526     
============================================
+ Hits          82274    84879    +2605     
- Misses        44911    45434     +523     
- Partials       6048     6299     +251     

Flag	Coverage Δ
custom-integration1	<0.01% <0.00%> (-0.01%)	⬇️
integration	<0.01% <0.00%> (-0.01%)	⬇️
integration1	<0.01% <0.00%> (-0.01%)	⬇️
integration2	0.00% <0.00%> (ø)
java-11	62.12% <91.45%> (+0.41%)	⬆️
java-21	27.94% <1.70%> (-33.69%)	⬇️
skip-bytebuffers-false	62.12% <91.45%> (+0.37%)	⬆️
skip-bytebuffers-true	27.94% <1.70%> (+0.21%)	⬆️
temurin	62.13% <91.45%> (+0.37%)	⬆️
unittests	62.12% <91.45%> (+0.37%)	⬆️
unittests1	46.68% <96.33%> (-0.21%)	⬇️
unittests2	27.94% <1.70%> (+0.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.

[klsince]

just loop over operands and add to the new list, w/o calling Arrays.asList() to avoid creating a temp list.

[Jackie-Jiang]

I actually considered that, but not sure if that is really faster. The trade-off is one extra array copy vs a lot of extra method invocations and a for loop. Seems IDE doesn't like the for loop, thus I decided to use the IDE suggested way