Skip to content

Pinot bugs and making it work with the latest trunk pinot#14057

Merged
highker merged 4 commits intoprestodb:masterfrom
agrawaldevesh:master
Feb 6, 2020
Merged

Pinot bugs and making it work with the latest trunk pinot#14057
highker merged 4 commits intoprestodb:masterfrom
agrawaldevesh:master

Conversation

@agrawaldevesh
Copy link
Contributor

@agrawaldevesh agrawaldevesh commented Feb 4, 2020

Three changes here: 1 medium and 2 tiny:

  • The nameToIndex used for the pinot column ordering map should not assume that the variable reference name is the same as the pinot lowercase name. It should be keyed off the keys in the selections map (in the pinot query generator context) and not the value. It was throwing an error with the query below because the output variables were named like player_initial_33, g_old_51 etc instead of just player_initial, g_old. This is because these columns are referenced twice in the query.
    This again smells like a core bug and it is totally my bad that I screwed this up the last time.
select * from (select yearid, substr(playerid, 1, 3) as player_initial, sum(homeruns) from pinot.baseballStats.baseballStats group by 1, 2) as m INNER join (select g_old, substr(playername, 1, 3) as player_name_initial, sum(numberofgames) from pinot.baseballStats.baseballStats group by 1, 2) as w ON m.player_initial = w.player_name_initial;
  • [tiny] Make this work with the new pinot trunk code which no longer likes "Content-Type: Application/json" for just GET requests and non body POSTS :-). As it shouldn't.
  • [tiny] renamed the config prefer broker queries to ! forbid broker queries to be true to the name
== RELEASE NOTES ==

Pinot Changes
*  Replace config `pinot.prefer-broker-queries` with the inverse config `pinot.forbid-broker-queries`.

@agrawaldevesh agrawaldevesh changed the title [WIP][Pinot] Fix bug with pinot column scan index generation [WIP] Pinot bugs and making it work with the latest trunk pinot Feb 4, 2020
@agrawaldevesh agrawaldevesh changed the title [WIP] Pinot bugs and making it work with the latest trunk pinot Pinot bugs and making it work with the latest trunk pinot Feb 4, 2020
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First commit: could you remove the "[Pinot]" flag from the commit title?

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second commit: could you update the title to be:
Rename preferBrokerQueries to its negate format forbidBrokerQueries

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3rd commit: could you update the title to be
Avoid passing Content-Type header when Pinot request body is empty

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit: could you update the title to be

Fail with informative error message if no Pinot expression is selected

@agrawaldevesh
Copy link
Contributor Author

James, thanks for the comments and careful review. As always !.

Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@highker highker merged commit a3f9aa3 into prestodb:master Feb 6, 2020
@caithagoras caithagoras mentioned this pull request Feb 20, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants