Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for MaxResults on Glue Hive Metastore #16012

Merged
merged 1 commit into from
May 19, 2021

Conversation

v-jizhang
Copy link
Contributor

@v-jizhang v-jizhang commented Apr 27, 2021

Cherry pick of trinodb/trino#3024 and
trinodb/trino#4938

Co-authored-by: Istvan [email protected]
Co-authored-by: Ashhar Hasan [email protected]

== RELEASE NOTES ==

Hive Changes
* Add support for MaxResults on Glue Hive Metastore

Copy link
Contributor

@aweisberg aweisberg left a comment

Choose a reason for hiding this comment

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

Hard coded values gives me slight pause, but it's in production in Trino and no one is complaining so I will +1 it.

This is metadata so I assume a constant metadata fetch size is acceptable although if it includes statistics it seems like it might be a value people want to change?

@aweisberg aweisberg requested a review from pettyjamesm May 12, 2021 19:59
@aweisberg
Copy link
Contributor

@pettyjamesm do you have an opinion on this being hard coded? Would you mind merging if you think it is acceptable.

@pettyjamesm
Copy link
Contributor

I saw this change in Trino after it had merged. I tried to reproduce the described behavior but what I saw seemed to indicate that setting this value actually has no effect and that there's some other implementation detail on the Glue end that actually determines how many partitions will be included in any given response. That said, hard coding a value should be harmless enough: I can't think of a use case that would want any less than the maximum partitions per response and the maximum allowed values for APIs like this tend not to change all that often.

@aweisberg
Copy link
Contributor

@pettyjamesm feel free to merge if you approve :-)

@pettyjamesm
Copy link
Contributor

@pettyjamesm feel free to merge if you approve :-)

The Facebook Integration step not being completed means that this PR hasn't met the "all checks must pass" criteria to merge. Is that something you can trigger on your side or will this PR need to be force-pushed to re-run?

@aweisberg
Copy link
Contributor

Updated to see if that will trigger an integration build. I think there is almost no chance it would do that, but not worth risking.

@pettyjamesm pettyjamesm merged commit 8f4ef27 into prestodb:master May 19, 2021
@pettyjamesm
Copy link
Contributor

Merged, thanks!

@sujay-jain sujay-jain mentioned this pull request May 21, 2021
10 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.

3 participants