the bad news is that I don't know how this is even happened in June. @User, as part of query context changes, was there ever a time where server briefly moved to PinotQuery (as part of conversion of broker request to QueryContext) and we mistakenly started using filter from PinotQuery

latest code doesn't reproduce this issue

I think we should just focus on:

1. Does the issue exist in prod - if so, mitigate/fix quickly.
2. Does the issue happen in latest code - fix forward

We don't need to debug old code

it doesn't happen

latest code or prod or both?

+100 to what Mayank said

latest code (in IntellIJ) after downloading segments and also on prod via pinot-query --sql --show-metadata

Do we have test that compares pql with sql filter? If not, perhaps we can add

we do

PqlSqlCompatibilityTest

does exactly this

it didn't fail for the above queries

ok

I see it does not have complex filter predicates. perhaps, add some

I think I might have fixed some bug on the SQL path and made it the same behavior as PQL

Any update here?

Yes, the issue does not exist in latest code

Cool

Can we add a test case?

Yes, either @User or I will do that

Apparently there is already one. But it likely does not have complex predicates. We just need to add a query

I will add tests tonight.

I would like to get an opinion on a slightly related topic. Query rewrite code has grown in non-trivial manner and I believe it will continue to grow. Can we add a fast path query compilation flag to bypass query rewrite? On the SQL path l, there are few different things happening in compilation 1. Calcite parsing to SQLNode 2. SQLNode to PinotQuery 3. PinotQuery rewrite.. 4. PinotQuery to BrokerRequest We should get rid of 4 very soon. In addition to that we should make 3 optional. Enabled by default. We can't do much about 1 except tuning config and making changes to calcite. 2 may have some room.. Request compilation metric used to stay close to 1ms or 900us for PQL. For SQL, I have seen it go up to 5ms.

Do you have the profile for 5ms?

What part of it is coming from rewrite

I can get the profile. It was an immediate thought after looking at the time and the code.

Ok, let’s have that. In general, I’d like to avoid configs that require tuning, or setting to get around some problem, we should find a way to automatically do that inside the code

5ms is definitely a lot

@User has left the channel