-
Notifications
You must be signed in to change notification settings - Fork 703
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
[CARBONDATA-1537] Fixed version compatabilty issues from V1 to latest carbon version #1398
Conversation
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/218/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/342/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/970/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/343/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/219/ |
Build Failed with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/220/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/971/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/344/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/972/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/345/ |
Build Failed with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/221/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/346/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/222/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/973/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/974/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/223/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/347/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/224/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/348/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/349/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/225/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/350/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/354/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/230/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/355/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/231/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/232/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/356/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/357/ |
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/233/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/985/ |
@@ -214,6 +218,42 @@ public void setNumberOfPages(int numberOfPages) { | |||
for (int i = 0; i < mSize; i++) { | |||
output.writeInt(measureChunksLength.get(i)); | |||
} | |||
// Serialize datachunks as well for older versions like V1 and V2 |
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.
Can you wrap this logic into a function and mentioning it is for V1 and V2 serialization only, I think it will be more readable
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.
ok
@@ -238,6 +278,20 @@ public void setNumberOfPages(int numberOfPages) { | |||
for (int i = 0; i < measureChunkOffsetsSize; i++) { | |||
measureChunksLength.add(input.readInt()); | |||
} | |||
|
|||
// Deserialize datachunks as well for older versions like V1 and V2 |
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.
Can you wrap this logic into a function and mentioning it is for V1 and V2 deserialization only, I think it will be more readable
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.
ok
// internal use only, for variable length data type | ||
BYTE_ARRAY(13, "BYTE_ARRAY", -1), | ||
|
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.
remove empty line
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.
ok
// internal use only, for value compression from integer/long to 3 bytes value | ||
SHORT_INT(14, "SHORT_INT", 3); | ||
SHORT_INT(14, "SHORT_INT", 3), | ||
// Only for internal use for backward compatability |
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.
mentioning it is for V1 and V2 format only
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.
ok
@@ -2070,6 +1994,20 @@ public static void dropDatabaseDirectory(String dbName, String storePath) | |||
} | |||
} | |||
|
|||
public static DataType getDataType(char type) { |
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.
Suggest to move all conversion to DataType enum, including this one and ColumnPageEncoderMeta.convertType
. And also move CarbonCommonConstants.BIG_INT_MEASURE
and related constants to DataType enum also.
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.
ok
@@ -123,6 +123,25 @@ private BlockletInfo getBlockletInfo( | |||
} | |||
|
|||
@Override public List<ColumnSchema> getSchema(TableBlockInfo tableBlockInfo) throws IOException { | |||
return null; | |||
FileHolder fileReader = null; |
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.
Is this change related to this PR? Can you add comment to describe the intention of this function in its abstract interface?
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.
yes, it is part of this PR. We need to get columnschema
import org.apache.carbondata.core.util.CarbonProperties | ||
|
||
/** | ||
* V1 to V3 compatability test. This test has to be at last |
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.
Will you add V2 to V3 compatibility test in future PR?
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.
yes, i will tests for V2 to V3 in another PR, Because I need to verify the compatibility of V2 to V3 first
@@ -43,7 +43,7 @@ class CarbonSession(@transient val sc: SparkContext, | |||
} | |||
|
|||
@transient | |||
override private[sql] lazy val sessionState: SessionState = new CarbonSessionState(this) | |||
override lazy val sessionState: SessionState = new CarbonSessionState(this) |
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.
Is this change required? Somewhere outside sql package will read this variable?
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.
Yes, it is required for test case I added.
Build Success with Spark 1.6, Please check CI http://88.99.58.216:8080/job/ApacheCarbonPRBuilder/294/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder1/418/ |
SDV Build Success , Please check CI http://144.76.159.231:8080/job/ApacheSDVTests/1048/ |
LGTM |
… carbon version This closes apache#1398
No description provided.