-
Notifications
You must be signed in to change notification settings - Fork 702
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-1018] Add unsafe ColumnPage implementation #1000
Conversation
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2232/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2237/ |
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2304/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2307/ |
Refer to this link for build results (access rights to CI server needed): |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2308/ |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2322/ |
apache:retest this please |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2324/ |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): Failed Tests: 1carbondata-pr-spark-1.6/org.apache.carbondata:carbondata-spark-common-test: 1 |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2334/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2452/ |
Refer to this link for build results (access rights to CI server needed): |
@@ -98,56 +117,54 @@ public static ColumnPage newPage(DataType dataType, int pageSize) { | |||
default: | |||
throw new RuntimeException("Unsupported data dataType: " + dataType); | |||
} | |||
instance.stats = new ColumnPageStatsVO(dataType); | |||
instance.nullBitSet = new BitSet(pageSize); | |||
return instance; | |||
} | |||
|
|||
protected static ColumnPage newBytePage(byte[] byteData) { | |||
ColumnPage columnPage = new ColumnPage(BYTE, byteData.length); |
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.
BytePage does not support unsafeColumnPage ?
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, I will fix
@@ -98,56 +117,54 @@ public static ColumnPage newPage(DataType dataType, int pageSize) { | |||
default: | |||
throw new RuntimeException("Unsupported data dataType: " + dataType); | |||
} | |||
instance.stats = new ColumnPageStatsVO(dataType); | |||
instance.nullBitSet = new BitSet(pageSize); | |||
return instance; | |||
} | |||
|
|||
protected static ColumnPage newBytePage(byte[] byteData) { | |||
ColumnPage columnPage = new ColumnPage(BYTE, byteData.length); |
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.
Byte doe not support unsafeColumnPage ?
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.
If don't support, we'd better to add some notes to explain.
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, it supports, I will fix
ColumnPage columnPage = new ColumnPage(BYTE_ARRAY, stringData.length); | ||
columnPage.byteArrayData = stringData; | ||
private static ColumnPage newVarLengthPage(byte[][] byteArray) { | ||
ColumnPage columnPage = new ColumnPage(BYTE_ARRAY, byteArray.length); |
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.
ByteArray does not support unsafeColumnPage ?
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.
It supports, I will update
@@ -508,7 +567,7 @@ public static ColumnPage decompress(Compressor compressor, DataType dataType, | |||
} | |||
|
|||
// input byte[] is LV encoded, this function can expand it into byte[][] | |||
private static byte[][] deflatten(byte[] input) { | |||
protected static byte[][] deflatten(byte[] input) { |
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 it be better to move deflatten to ByteUtil which is same with flatten ?
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.
Agree with erlu.
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
private void ensureMemory(int requestSize) throws MemoryException { | ||
if (totalLength + requestSize > capacity) { | ||
memoryBlock = UnsafeMemoryManager.reallocateMemoryWithRetry( | ||
memoryBlock, (long)((totalLength + requestSize) * FACTOR)); |
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.
capacity
should be updated with (totalLength + requestSize) * FACTOR
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
CarbonUnsafe.unsafe.copyMemory(bytes, CarbonUnsafe.BYTE_ARRAY_OFFSET + offset, | ||
baseAddress, baseOffset + rowOffset[rowId], length); | ||
} | ||
|
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.
Even method public void putBytes(int rowId, byte[] bytes)
also need to be override here.
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.
It is implemented in VarLengthColumnPageBase
and it will call putBytesAtRow
abstract method
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 see that. I mean we should do in unsafe
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2491/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2494/ |
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2509/ |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2521/ |
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2526/ |
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2528/ |
retest this please |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2546/ |
Refer to this link for build results (access rights to CI server needed): |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2551/ |
Refer to this link for build results (access rights to CI server needed): Failed Tests: 2carbondata-pr-spark-1.6/org.apache.carbondata:carbondata-spark-common-test: 2 |
retest this please |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2558/ |
Refer to this link for build results (access rights to CI server needed): Failed Tests: 1carbondata-pr-spark-1.6/org.apache.carbondata:carbondata-spark-common-test: 1 |
Build Failed with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder/2574/ |
Refer to this link for build results (access rights to CI server needed): |
LGTM |
This PR is based on #987
In this PR, an
UnsafeColumnPage
is added, it can reduce memory requirement for data loading.Before loading, user can add following property to enable this feature