Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Iceberg] Add Histogram Statistic Support #22365
base: master
Are you sure you want to change the base?
[Iceberg] Add Histogram Statistic Support #22365
Changes from all commits
c3b8502
be39184
dfe82f6
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's attribute this to Guava ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we just use Guava for all of this? We already depend on it. If there is some reason we want to fork this code, then full tests are needed. But it should be much easier just to call Guava
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The guava dependency is not included in
presto-common
as thepresto-common
module is a dependency of thepresto-spi
module which we avoid putting additional dependencies so that when creating connectors or other plugins there it is less likely for downstream users of presto to encounter issues or conflictsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's reasonable. However if we're forking this in, we also MUST include the tests as well, and be prepared to support this for the foreseeable future. I'm not sure it's worth the effort. I'd be inclined to just manually build the strings in the toString method like we've been doing since 1995. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we really need guava without disrupting the downstream users of the library - we could consider shading it (during the build process)? or even create a separate library e.g. presto-shaded-guava. There is an extra effort in testing and maintaining this code.
@ZacBlanco is this already considered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, let's not do that. Shading is so painful. I think I'm coming down pretty firmly on the side of don't use ToStringHelper, don't use Guava here, and just concatenate the strings inside toString. It's not like that's actually hard. This approach is way too complicated for the problem it solves.
https://jlbp.dev/JLBP-18
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ScrapCodes, as @elharo mentioned, shading is definitely a last resort and we should avoid it as much as possible. It's not worth doing. Guava is a huge codebase and would add a lot of bloat just for a few classes. I think vendoring these small utilities are useful though