Apache CarbonData Dev Mailing List archive › Apache CarbonData JIRA issues

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

Classic

List

Threaded

76 messages Options

1234

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

Github user CarbonDataQA commented on the issue:

https://github.com/apache/carbondata/pull/2872

Build Failed with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9656/

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user CarbonDataQA commented on the issue:

https://github.com/apache/carbondata/pull/2872

Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1608/

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user CarbonDataQA commented on the issue:

https://github.com/apache/carbondata/pull/2872

Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1417/

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user CarbonDataQA commented on the issue:

https://github.com/apache/carbondata/pull/2872

Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1626/

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user CarbonDataQA commented on the issue:

https://github.com/apache/carbondata/pull/2872

Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9674/

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user ravipesala commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234125610

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/ReusableDataBuffer.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.carbondata.core.datastore;
+
+public class ReusableDataBuffer {
--- End diff --

Please add comments to this class

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user manishgupta88 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234176956

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/chunk/reader/MeasureColumnChunkReader.java ---
@@ -56,13 +57,14 @@ MeasureRawColumnChunk readRawMeasureChunk(FileReader fileReader, int columnIndex
* @return
* @throws IOException
*/
- ColumnPage decodeColumnPage(MeasureRawColumnChunk measureRawColumnChunk,
- int pageNumber) throws IOException, MemoryException;
+ ColumnPage decodeColumnPage(MeasureRawColumnChunk measureRawColumnChunk, int pageNumber,
+ ReusableDataBuffer reusableDataBuffer) throws IOException, MemoryException;

/**
* Decode raw data and fill the vector
*/
- void decodeColumnPageAndFillVector(MeasureRawColumnChunk measureRawColumnChunk,
- int pageNumber, ColumnVectorInfo vectorInfo) throws IOException, MemoryException;
+ void decodeColumnPageAndFillVector(MeasureRawColumnChunk measureRawColumnChunk, int pageNumber,
+ ColumnVectorInfo vectorInfo, ReusableDataBuffer reusableDataBuffer)
+ throws IOException, MemoryException;
--- End diff --

Can we have separate interface methods with and wothout `ReusableDataBuffer` for both the above methods. Many places null is being passed which can be avoided using separate interface methods and then the common code can be refactored for classes implementing these methods

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user manishgupta88 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234201988

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/compression/ZstdCompressor.java ---
@@ -74,4 +74,16 @@ public long maxCompressedLength(long inputSize) {
public boolean supportUnsafe() {
return false;
}
+
+ @Override public int unCompressedLength(byte[] data, int offset, int length) {
+ throw new RuntimeException("Unsupported operation Exception");
+ }
+
+ @Override public int rawUncompress(byte[] data, int offset, int length, byte[] output) {
+ throw new RuntimeException("Unsupported operation Exception");
+ }
+
+ @Override public boolean supportReusableBuffer() {
+ return false;
--- End diff --

Move the default implementation to Abstract class `AbstractCompressor`

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user jackylk commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234209700

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/ReusableDataBuffer.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.carbondata.core.datastore;
+
+public class ReusableDataBuffer {
--- End diff --

add interface annotation also

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user jackylk commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234209828

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/ReusableDataBuffer.java ---
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.carbondata.core.datastore;
+
+public class ReusableDataBuffer {
+
+ private byte[] dataBuffer;
+
+ private int size;
+
+ public byte[] getDataBuffer(int requestedSize) {
--- End diff --

add comment

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user manishgupta88 commented on the issue:

https://github.com/apache/carbondata/pull/2872

@kumarvishal09 ...In general, instead of passing the `ReusableDataBuffer` instance everwhere can we make this class singleton and create a `byte[]` pool and use the concept of object pool. This will help in following ways:
1. It will avoid modification of too many method/code modifications as wherever required we can take the object from pool and make the code simpler.
2. Currentlt it is being used for filling hthe decompressed data to avoid array copy. In future it will be easier to make use of the reusable buffer object pool wherever required without making too many changes in the code.
We can discuss more on this and decide.

---

qiuchenjian-2

[GitHub] carbondata pull request #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user akashrn5 commented on a diff in the pull request:

https://github.com/apache/carbondata/pull/2872#discussion_r234233366

--- Diff: core/src/main/java/org/apache/carbondata/core/datastore/page/encoding/adaptive/AdaptiveIntegralCodec.java ---
@@ -119,24 +120,34 @@ public ColumnPage decode(byte[] input, int offset, int length)
return LazyColumnPage.newPage(page, converter);
}

- @Override
- public void decodeAndFillVector(byte[] input, int offset, int length,
- ColumnVectorInfo vectorInfo, BitSet nullBits, boolean isLVEncoded)
- throws MemoryException, IOException {
- ColumnPage page = null;
- if (DataTypes.isDecimal(meta.getSchemaDataType())) {
- page = ColumnPage.decompressDecimalPage(meta, input, offset, length);
- vectorInfo.decimalConverter = ((DecimalColumnPage) page).getDecimalConverter();
+ @Override public void decodeAndFillVector(byte[] input, int offset, int length,
+ ColumnVectorInfo vectorInfo, BitSet nullBits, boolean isLVEncoded, int pageSize,
+ ReusableDataBuffer reusableDataBuffer) throws MemoryException, IOException {
+ Compressor compressor =
+ CompressorFactory.getInstance().getCompressor(meta.getCompressorName());
+ byte[] unCompressData;
+ if (null != reusableDataBuffer && compressor.supportReusableBuffer()) {
+ int uncompressedLength = compressor.unCompressedLength(input, offset, length);
+ unCompressData = reusableDataBuffer.getDataBuffer(uncompressedLength);
} else {
- page = ColumnPage.decompress(meta, input, offset, length, isLVEncoded);
+ unCompressData = compressor.unCompressByte(input, offset, length);
}
- page.setNullBits(nullBits);
- converter.decodeAndFillVector(page, vectorInfo);
+ compressor.rawUncompress(input, offset, length, unCompressData);
--- End diff --

i think, when ZSTD compressor is configured, it will throw unsupported exception, plese check and handle for `AdaptiveIntegralCodec` and `AdaptiveDeltaIntegralCodec`

---

qiuchenjian-2

[GitHub] carbondata issue #2872: [WIP] Added reusable buffer code

In reply to this post by qiuchenjian-2

Github user kumarvishal09 commented on the issue:

https://github.com/apache/carbondata/pull/2872

@manishgupta88 this is basic implementation of pool(kind of pool :)) and currently it only for direct flow(new flow), In future I am planing to replace reusable buffer with proper byte array pooling which can be used for both old and new flow. I will raise a JIRA for the same

---

qiuchenjian-2