In the initial version, I'd skipped the optimize step here for perf reasons but turns out that there are some cases where bad queries are only caught in the optimize step. For instance, this test started failing because the bad query compiles fine but the PinotEvaluateLiteralRule catches the issue during the optimize query phase.

Can we also add a config to sample the queries we are going to compare. I mean, instead of compiling every query in V2, we can say something like: compile 1 every X queries or a X% of the queries.

Given the high QPS Pinot usually has, this sampling should be good enough to provide a good enough overview without having to parse all queries in V2, which may add some impact.

Alternatively we can delegate this into a background thread that may be reading from a concurrent queue, so running V1 queries won't be affected by the time spent in planning V2. The important part here is that the thread adding into the queue should be using Queue.offer so if the queue is full it just continues (the query won't be reported, but at the same time it won't be blocked).

Thanks for the suggestions! I do agree that we need to be careful about affecting query latency in the v1 engine due to this additional query compilation overhead. I liked your second option more because the first option can still introduce unpredictable latency in some queries which can increase the tail latency by a fair amount. I did some ad-hoc benchmarking and for some queries, the v2 engine query compilation time is actually comparable in order of magnitude to the overall v1 query execution time (and I'm sure in certain cases it could even be higher).

Also, I'm not a big fan of adding more user configuration than is necessary. I've gone with the second option for now - background thread processing (v2 query compiling) a queue of v1 query requests where the queue capacity is currently hardcoded / not configurable. We could make this queue capacity configurable in the future if we see a need for allowing this to be configurable by users - i.e., in order to tune the percentage of requests that are effectively sampled based on the query workload.

I've gone with a simple single threaded executor and a blocking queue implementation for now, but on second thoughts I think we might run into contention issues in high QPS environments with the request processing threads all vying for the blocking queue's internal lock during the call to offer. I think we could potentially solve this using our own additional external lock and just drop requests if the lock isn't free (using Lock.tryLock). Although this would still introduce some additional overhead. Thoughts?

Alternatively, we could potentially use a lock-free bounded queue (like BoundedFifoBuffer from commons-collections) with some additional logic on our end to avoid busy waiting on the consumer side when the buffer is empty. Or, we could use Java's ConcurrentLinkedQueue which is lock-free but unbounded and add some logic on our end to make it bounded (and also the logic for avoiding busy waiting on the consumer side when the queue is empty). Neither option is too appealing though 😄

Edit: for the waiting logic we'd need our own external lock in any case, so we could just wrap a regular non-synchronized lock-free bounded queue.

I don't think that lock will be that problematic. Given this feature is behind a config flag, it can be enabled as needed. In case ends up being a performance issue we can just turn it off.

Thanks for the suggestions! I do agree that we need to be careful about affecting query latency in the v1 engine due to this additional query compilation overhead. I liked your second option more because the first option can still introduce unpredictable latency in some queries which can increase the tail latency by a fair amount. I did some ad-hoc benchmarking and for some queries, the v2 engine query compilation time is actually comparable in order of magnitude to the overall v1 query execution time (and I'm sure in certain cases it could even be higher).

Also, I'm not a big fan of adding more user configuration than is necessary. I've gone with the second option for now - background thread processing (v2 query compiling) a queue of v1 query requests where the queue capacity is currently hardcoded / not configurable. We could make this queue capacity configurable in the future if we see a need for allowing this to be configurable by users - i.e., in order to tune the percentage of requests that are effectively sampled based on the query workload.