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
ORC-562:Don't wrap readerSchema in acidSchema, if readerSchema is alr… #442
Conversation
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.
You really need to add a test case as well.
@@ -87,7 +87,8 @@ public SchemaEvolution(TypeDescription fileSchema, | |||
this.fileSchema = fileSchema; | |||
this.isAcid = checkAcidSchema(fileSchema); | |||
this.includeAcidColumns = options.getIncludeAcidColumns(); | |||
this.readerColumnOffset = isAcid ? acidEventFieldNames.size() : 0; | |||
this.readerColumnOffset = (isAcid && readerSchema != null && checkAcidSchema(readerSchema)) || !isAcid |
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'd suggest keeping the checkAcidSchema(readerSchema) rather than calling it multiple times. This expression needs to be simplified.
@@ -87,7 +87,8 @@ public SchemaEvolution(TypeDescription fileSchema, | |||
this.fileSchema = fileSchema; | |||
this.isAcid = checkAcidSchema(fileSchema); | |||
this.includeAcidColumns = options.getIncludeAcidColumns(); | |||
this.readerColumnOffset = isAcid ? acidEventFieldNames.size() : 0; | |||
this.readerColumnOffset = (isAcid && readerSchema != null && checkAcidSchema(readerSchema)) || !isAcid | |||
? 0 : acidEventFieldNames.size(); | |||
if (readerSchema != null) { | |||
if (isAcid) { |
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.
You probably want to cut out here with if (isAcid && !isReaderAcid) rather than change addEventSchema.
@omalley I changed the implementation and added new test. |
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.
Thank you!
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!
…eady acid Fixes #442 Signed-off-by: Owen O'Malley <omalley@apache.org>
I've committed this. |
…eady acid Fixes apache#442 Signed-off-by: Owen O'Malley <omalley@apache.org>
…hema is already acid Fixes apache#442 Signed-off-by: Owen O'Malley <omalley@apache.org> (cherry picked from commit 2db32a8) Change-Id: Icfc548d19b0d0cc62b3e0e90ea164c791ece0a94
…eady acid Fixes apache#442 Signed-off-by: Owen O'Malley <omalley@apache.org>
…eady acid