-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-27686 ORC upgraded to 1.8.5. #4690
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -848,7 +848,7 @@ public void testCompactStatsGather() throws Exception { | |
| .getParameters(); | ||
| Assert.assertEquals("The number of files is differing from the expected", "1", parameters.get("numFiles")); | ||
| Assert.assertEquals("The number of rows is differing from the expected", "4", parameters.get("numRows")); | ||
| Assert.assertEquals("The total table size is differing from the expected", "704", parameters.get("totalSize")); | ||
| Assert.assertEquals("The total table size is differing from the expected", "705", parameters.get("totalSize")); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I saw you have removed
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be better to handle this in a separate ticket as refactoring. Would this be okay so we can close this ticket?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we're removing some totalSize assertions but not others, let's simply keep it consistent, choose 1 option now from the below ones:
I believe only 1) makes sense
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just unresolved this conversation, it's time to finally address this whole totalSize somehow
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Option 5 means we remove only the totalSize checks which are in this ticket and not others, which would be a little bit weird, because it is hard to address which to eliminate outside this ticket. How can be the other ticket reviewed without this? So I would say create a ticket with clear description which to delete: a) from Qtests, b) Qtest and unit tests. Or create 2 new ticket, one for Qtests only, one for unit tests only. Since the build env is really unstable I had to run many times this build to finally succeed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. okay, makes sense, so we can agree to remove totalSize checks here from every place that were affected by this ORC upgrade and do the rest in subsequent patches, like HIVE-27791 |
||
| } | ||
|
|
||
| @Test | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| --! qt:replace:/(totalSize\s+)(\S+|\s+|.+)/$1#Masked#/ | ||
| -- create table | ||
| -- numeric type | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.