HIVE-27686 ORC upgraded to 1.8.5.#4690
Conversation
asf-ci-hive
added
tests pending
tests failed
and removed
tests pending
tests failed
labels
asf-ci-hive
added
tests failed
tests pending
and removed
tests pending
tests failed
labels
asf-ci-hive
added
tests pending
tests failed
tests unstable
tests passed
and removed
tests failed
tests pending
tests unstable
labels
aturoczy
left a comment
aturoczy
left a comment
There was a problem hiding this comment.
Please remove the unnecessary Asserts
Change-Id: I9cef3ce5e91819ef2d2c169276aac96bcf0f80c8
|
Kudos, SonarCloud Quality Gate passed!
|
zhangbutao
left a comment
zhangbutao
left a comment
There was a problem hiding this comment.
Thanks for the change! IMO, we can remove totalSize in every UT&QTest as its value would change in different ORC version, and it is not a very important element for ORC UT.
| 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")); |
There was a problem hiding this comment.
I saw you have removed totalSize in the UT of TestCompactor.java, so should we remove this line as well?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
we're removing some totalSize assertions but not others, let's simply keep it consistent, choose 1 option now from the below ones:
- remove all occurrences that have been changed here by the patch, so it's visible now
- remove only from qtests
- remove all occurrences from the code -> time-consuming, I'm not recommending this
- don't remove any totalSize assertions
I believe only 1) makes sense
There was a problem hiding this comment.
I just unresolved this conversation, it's time to finally address this whole totalSize somehow
option 5) would be to create another ticket to remove that stuff, merge ASAP, and then we can see how clear is this upgrade alone, because masking/totalSize related changes won't bring the noise into this PR, so I guess this is even better than 1)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
6 participants
What changes were proposed in this pull request?
ORC is upgraded to use 1.8.5 which contains a fix to use ORC row level filter.The tez.grouping.min-size needed to be changed to have 4 buckets for compaction testing.
Why are the changes needed?
To be able to use ORC row level filter.
Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
How was this patch tested?
Manually.