[GitHub] [pinot] Jackie-Jiang commented on pull request #8979: optimize `order by sorted ASC, unsorted` and `order by DESC` cases

[GitBox <gi...@apache.org>]

Jackie-Jiang commented on PR #8979:
URL: https://github.com/apache/pinot/pull/8979#issuecomment-1189551878

   > I think I finally found why the test was failing. It was due to a `cmp >= 0` assertion that should be `cmp <= 0`. The reason that made it very hard to find the problem is that `BaseCombineOperator` is not prepared to catch `Errors`, so the task at the server just failed without printing any log and as a result the broker didn't receive a response, so the error shown in the broker was that some server didn't answer.
   > 
   > Things to know about:
   > 
   > * Pinot may not log `AssertionErrors`, so it is better to do not use `assert` expressions
   > * We may or may not want to log in case we receive an error. As I don't know what do we want to do in that case, I've just changed my code to throw an `ArgumentException`.
   
   We use `assert` to ensure certain behavior, but also avoid the overhead of the extra check. In production environment, `assert` statement is ignored, which is the desired behavior. In performance critical path, we want to avoid the extra check if we know certain behavior is expected.
   `assert` can be enabled by setting a JVM parameter: `-ea`


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org