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
PARQUET-1115: Warn users when misusing parquet-tools merge #433
Conversation
public String getCommandDescription() { | ||
return "Merges multiple Parquet files into one. " + | ||
"The command doesn't merge row groups, just places one after the other. " + | ||
"When used to merge many small files, the resulting file will still contain small row groups."; |
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.
I would add ", which usually leads to bad query performance."
|
||
if (tooSmallFilesMerged) { | ||
out.println("Warning: you merged too small files. " + | ||
"Despite the size of the merged file is bigger, it STILL contains small row groups, thus you don't have the advantage of big row groups!"); |
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.
I would use stronger wording after the last comma, like "which usually leads to bad query performance." (Additionally, I'm not a native speaker either, but I believe you either need a "that" after "Despite" or use some other word, for example "Although".)
@zivanfi thanks for you review, changed the wording accordingly. |
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.
LGTM, thanks!
I'll wait a few days to give everyone a chance to comment, then I'll commit it.
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.
Nandor, could you please remove whitespace changes from unrelated lines? They can cause problems in the future when cherry-picking/merging/rebasing/blaming. Thanks!
@zivanfi removed the whitespace-only changes |
Author: Nandor Kollar <nkollar@cloudera.com> Closes #433 from nandorKollar/PARQUET-1115 and squashes the following commits: 5504a39 [Nandor Kollar] PARQUET-1115: Warn users when misusing parquet-tools merge f2ece26 [Nandor Kollar] PARQUET-1115: Warn users when misusing parquet-tools merge 4f3ec99 [Nandor Kollar] PARQUET-1115: Warn users when misusing parquet-tools merge f97e620 [Nandor Kollar] PARQUET-1115: Prevent users from misusing parquet-tools merge
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.
Thanks Nándor, I merged your change into the codebase.
It might be nice if we actually suggested an alternative instead of just saying "don't do this." You can see my solution at https://gist.github.com/NickCrews/7a47ef4083160011e8e533531d73428c. |
No description provided.