Adding Pinot SQL endpoint support

[xiangfu0]

The motivation of this PR to add Pinot SQL endpoint support is that Pinot Community is moving to SQL syntax/endpoint on query side to support more features and also plan to deprecate PQL queries.

This PR:

• Support Pinot new SQL endpoint for broker queries.
• Enable this feature by settingpinot.use-pinot-sql-for-broker-queries=true.
• Support aggregation/group by/oder by pushdown to query Pinot broker.

== RELEASE NOTES ==

Pinot Changes
* Add Pinot SQL endpoint support.

[agrawaldevesh]

Awesome improvement ! Can you please add some more details about the design/approach in the PR description. It would help with reviewing since the PR is rather large. Thanks.

[agrawaldevesh]

haven't looked at the tests

[agrawaldevesh]

I think some of the comments are still yet to be addressed ? Also, it would be great to call out in the PR description what the general approach was and what parts are like semantically new vs just code movement.

Thanks ! (Getting really really close !)

[agrawaldevesh]

Some more comments on beefing up the test coverage to cover the salient differences b/w the SQL and the PQL end points.

[agrawaldevesh]

some questions on your responses please :-)

Also, I think we should update the PR description to add more motivation on why we are interested in moving to the SQL end point. Perhaps you can say that the PQL end point is going to be deprecated or something to that effect ?

Thanks !

[agrawaldevesh]

Much much better testing !

A few nits on comments on stuff.

But my biggest remaining question is around my lack of understanding of the new outputs field in the PinotQueryGeneratorContext: Why it can't be used for PQL and the special handling needed for it in visitProject. Please see the comments inline.

[agrawaldevesh]

Just like line 319 above, should this also be Map<> newSelections = new HashMap<> ? (instead of new LinkedHashMap)

[xiangfu0]

Here the reason is that in our PQL generation test, we create AggregationPlanNode by setting the Aggregations directly, if we use HashMap then generatedPql cannot guarantee the ordering of aggregation functions. Otherwise we need to set a ProjectNode always outside an AggregationPlanNode

[agrawaldevesh]

Lets please fix the tests instead of complicating the production code like this. Even if it means more changes.

[xiangfu0]

will do :)

[xiangfu0]

The issue is that in visitAggregation method, I didn't explicitly generate outputs, and infer it when calling context.withAggregation by using outputs.addAll(newSelection.keySet()). This is fine when selection is an ordered map.
So the change I made is to generate outputs in visitAggregation method.

[agrawaldevesh]

Starting to look really good. Can you also please mark the resolved comments as such. Its getting hard to review the incremental changes esp because Github doesn't deal with force pushes that nicely :-)

I think my only remaining comment here is about not having to use a LinkedHashMap for the selections just to make the test code pass. I would rather we change the test code.

[xiangfu0]

Starting to look really good. Can you also please mark the resolved comments as such. Its getting hard to review the incremental changes esp because Github doesn't deal with force pushes that nicely :-)

I think my only remaining comment here is about not having to use a LinkedHashMap for the selections just to make the test code pass. I would rather we change the test code.

The issue is that in visitAggregation method, I didn't explicitly generate outputs, and infer it when calling context.withAggregation by using outputs.addAll(newSelection.keySet()). This is fine when selection is an ordered map.
So the change I made is to generate outputs in visitAggregation method.

The best of it is NO test code changes!

[agrawaldevesh]

Just last few nits. LGTM otherwise !

[xiangfu0]

Just last few nits. LGTM otherwise !

Many many thanks!!!

[agrawaldevesh]

@highker, @sachdevs please help review this PR.

@fx19880617 and I have done several passes over this and I am pretty satisfied with the end result: The necessary refactoring needed to support both PQL and SQL is clean, in that it tries to share maximal code and the new abstractions introduced also make more sense. There is great test coverage to boot.

Thanks.

[highker]

@zhenxiao, could you help to take a look at the PR also?

[highker]

@fx19880617, can we squash all commits into one?

[highker]

lgtm

[zhenxiao]

nice work @fx19880617
looks good to me
a few small things

[zhenxiao]

is it possible we make counter a private data member, and return counter here?

[agrawaldevesh]

So the problem is that the Broker page source is one shot: It does not have a notion of partial or a cursor: Either the number of completed positions is zero or it is the entire number of rows being returned.

The current usage of completed positions (if you search for the callers of this function), is to basically show a progress indicator while the query is running. That does not fit the broker page source semantics: Because the counter will only change basically when the query is finished (ie when the counter is numRows, then the page source is already done and the query is being torn down)

[zhenxiao]

I think counter is completed positions?

[agrawaldevesh]

replied above.

[zhenxiao]

could outputs be Set? LinkedHashSet is an implementation

[agrawaldevesh]

I would very much like to keep it as a LinkedHashSet ... I can't tell you the number of times this has bit me: If order is needed then it should be statically checked.

[highker]

@fx19880617, could you squash the commits and push? I will merge afterwards.

[xiangfu0]

@fx19880617, could you squash the commits and push? I will merge afterwards.

Done! Many Thanks!!!