[CARBONDATA-2927] multiple issue fixes for varchar column and complex columns, row that grows more than 2MB #2706

ajantha-bhat · 2018-09-10T18:07:25Z

1. varchar data length is more than 2MB, buffer overflow exception (thread local row buffer)
root casue*: thread* loaclbuffer was hardcoded with 2MB.
solution: grow dynamically based on the row size.

2. read data from carbon file having one row of varchar data with 150 MB length is very slow.
root casue: At UnsafeDMStore, ensure memory is just incresing by 8KB each time and lot of time malloc and free happens before reaching 150MB. hence very slow performance.
solution: directly check and allocate the required size.

3. Jvm crash when data size is more than 128 MB in unsafe sort step.
root cause: unsafeCarbonRowPage is of 128MB, so if data is more than 128MB for one row, we access block beyond allocated, leading to JVM crash.
solution: validate the size before access and prompt user to increase unsafe memory. (by carbon property)

Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:

Any interfaces changed? NA
Any backward compatibility impacted? NA
Document update required? NA
Testing done
done
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA. NA

CarbonDataQA · 2018-09-10T18:11:28Z

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

CarbonDataQA · 2018-09-10T18:13:45Z

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

CarbonDataQA · 2018-09-10T18:14:24Z

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

ajantha-bhat · 2018-09-11T16:07:50Z

@kumarvishal09 @ravipesala : please do in-depth review for this PR. impact is more.

CarbonDataQA · 2018-09-11T16:19:15Z

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

CarbonDataQA · 2018-09-11T16:29:47Z

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

CarbonDataQA · 2018-09-11T16:31:21Z

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

xuchuanyin · 2018-09-12T03:04:32Z

core/src/main/java/org/apache/carbondata/core/memory/UnsafeMemoryManager.java

@@ -200,7 +200,7 @@ public static MemoryBlock allocateMemoryWithRetry(long taskId, long size)
    }
    if (baseBlock == null) {
      INSTANCE.printCurrentMemoryUsage();
-      throw new MemoryException("Not enough memory");
+      throw new MemoryException("Not enough memory, increase carbon.unsafe.working.memory.in.mb");


I think you can optimize the error message to
Not enough unsafe working memory (total: , available: , request: )

xuchuanyin · 2018-09-12T03:07:25Z

processing/src/main/java/org/apache/carbondata/processing/loading/sort/SortStepRowHandler.java

@@ -559,7 +572,13 @@ public int writeRawRowAsIntermediateSortTempRowToUnsafeMemory(Object[] row,
    return size;
  }

-
+  private void validateUnsafeMemoryBlockSizeLimit(long unsafeRemainingLength, int size)


please optimize the parameter name of 'size' for better reading, it seems that it represents the requestedSize

xuchuanyin · 2018-09-12T03:09:36Z

processing/src/main/java/org/apache/carbondata/processing/loading/sort/SortStepRowHandler.java

      rowBuffer.putInt((int) row[this.dictNoSortDimIdx[idx]]);
    }
    // convert no-dict & no-sort
    for (int idx = 0; idx < this.noDictNoSortDimCnt; idx++) {
      byte[] bytes = (byte[]) row[this.noDictNoSortDimIdx[idx]];
+      // cannot exceed default 2MB, hence no need to call ensureArraySize


for one column, it may not exceed 2MB, what if we lots of no-sort-no-dict columns?

2 MB is not enough for varchar and complex complex columns.

xuchuanyin · 2018-09-12T03:11:23Z

processing/src/main/java/org/apache/carbondata/processing/loading/sort/SortStepRowHandler.java

      rowBuffer.putShort((short) bytes.length);
      rowBuffer.put(bytes);
    }
    // convert varchar dims
    for (int idx = 0; idx < this.varcharDimCnt; idx++) {
      byte[] bytes = (byte[]) row[this.varcharDimIdx[idx]];
+      // can exceed default 2MB, hence need to call ensureArraySize
+      rowBuffer = UnsafeSortDataRows


Should we call this method per row per column?
Since in most scenarios, 2MB per row is enough, so will the method calling here cause performance decrease?

xuchuanyin · 2018-09-12T03:11:50Z

processing/src/main/java/org/apache/carbondata/processing/loading/sort/SortStepRowHandler.java

@@ -598,26 +625,53 @@ private void packNoSortFieldsToBytes(Object[] row, ByteBuffer rowBuffer) {
      tmpValue = row[this.measureIdx[idx]];
      tmpDataType = this.dataTypes[idx];
      if (null == tmpValue) {
+        // can exceed default 2MB, hence need to call ensureArraySize
+        rowBuffer = UnsafeSortDataRows
+            .ensureArraySize(1);


bad indent, can be moved to previous line

xuchuanyin · 2018-09-12T03:13:44Z

.../src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeCarbonRowPage.java

@@ -59,12 +60,11 @@ public UnsafeCarbonRowPage(TableFieldStat tableFieldStat, MemoryBlock memoryBloc
    this.taskId = taskId;
    buffer = new IntPointerBuffer(this.taskId);
    this.dataBlock = memoryBlock;
-    // TODO Only using 98% of space for safe side.May be we can have different logic.
-    sizeToBeUsed = dataBlock.size() - (dataBlock.size() * 5) / 100;
+    sizeToBeUsed = dataBlock.size();


Is the old comment outdated? Have you ensured the 'safe side' it mentioned?

Please keep back this code, it is for reserving memory for a row in case it exceeds

xuchuanyin · 2018-09-12T03:15:32Z

...g/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java

@@ -72,7 +72,7 @@

  private SortParameters parameters;
  private TableFieldStat tableFieldStat;
-  private ThreadLocal<ByteBuffer> rowBuffer;
+  private static ThreadLocal<ByteBuffer> rowBuffer;


I think the 'static' here may cause problem for concurrent loading. Each loading should their own rowBuffer.

xuchuanyin · 2018-09-12T03:16:42Z

...g/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java

      }
+    } catch (Exception e) {
+      LOGGER


bad indent. we can move the msg to next line and keep method call in this line

xuchuanyin · 2018-09-12T03:17:34Z

...g/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java

@@ -326,6 +335,19 @@ private void startFileBasedMerge() throws InterruptedException {
    dataSorterAndWriterExecutorService.awaitTermination(2, TimeUnit.DAYS);
  }

+  public static ByteBuffer ensureArraySize(int requestSize) {


please give a comment that this method is used to increase the rowbuffer during loading.

xuchuanyin · 2018-09-12T03:19:23Z

...g/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java

@@ -326,6 +335,19 @@ private void startFileBasedMerge() throws InterruptedException {
    dataSorterAndWriterExecutorService.awaitTermination(2, TimeUnit.DAYS);
  }

+  public static ByteBuffer ensureArraySize(int requestSize) {


If we increase the rowbuffer runtime, is there a way to decrease it? Or if there is no need to do so, how long will this rowbuffer last?

xuchuanyin · 2018-09-12T03:26:17Z

@ajantha-bhat
Hi, I think the main problem may be that you set the 'rowbuffer' as static which should not be shared among different data loadings.

Besides, the judgement for increasing rowBuffer size per row per column may decrease data loading performance.

As a result, I'd like to implement this in an easier way.

We can add a table propery or load option for the size of row buffer. Just keep the previous row-buffer related code as it is. All you need is to change the initial size of the rowbuffer based on the table property or load option.

@kumarvishal09 @ravipesala How do you think?

ravipesala · 2018-09-12T14:14:08Z

...g/src/main/java/org/apache/carbondata/processing/loading/sort/unsafe/UnsafeSortDataRows.java

+  }
+
+  static  {
+    rowBuffer = new ThreadLocal<ByteBuffer>() {


Please try using DataOutSteam backed by ByteOutPutStream, it can expand dynamically.

CarbonDataQA · 2018-09-17T07:51:20Z

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

CarbonDataQA · 2018-09-17T08:36:04Z

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

CarbonDataQA · 2018-09-17T08:44:17Z

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

xuchuanyin · 2018-09-17T08:44:45Z

...sing/src/main/java/org/apache/carbondata/processing/store/CarbonFactDataHandlerColumnar.java

@@ -239,6 +239,7 @@ public void addDataToStore(CarbonRow row) throws CarbonDataWriterException {
   * @return false if any varchar column page cannot add one more value(2MB)
   */
  private boolean isVarcharColumnFull(CarbonRow row) {
+    //TODO: test and remove this as now  UnsafeSortDataRows can exceed 2MB


This is for a column page cannot exceed 2GB （actually it is 1.67GB since snappy cannot compress a bigger size in one run）, so there is no need to add a comment here

row with complex column also can grow very big (so checking only for var char is not good)
Also now rows grow more than 2MB. so, we need to modify this check.
This can be handled in separate PR.

now no impact from this method as "if 2MB itself space not there, more than 2MB space will never be there". so functionality remains same.

CarbonDataQA · 2018-09-17T10:21:51Z

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

CarbonDataQA · 2018-09-17T10:24:15Z

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

CarbonDataQA · 2018-09-17T10:24:19Z

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

CarbonDataQA · 2018-09-17T11:39:36Z

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

ravipesala · 2018-09-17T11:46:00Z

core/src/main/java/org/apache/carbondata/core/util/ReUsableByteArrayDataOutputStream.java

+    this.outputStream = outputStream;
+  }
+
+  public void resetByteArrayOutputStream() {


Just change name to reset

ravipesala · 2018-09-17T11:46:23Z

core/src/main/java/org/apache/carbondata/core/util/ReUsableByteArrayDataOutputStream.java

+    outputStream.reset();
+  }
+
+  public int getByteArrayOutputStreamSize() {


Just change name to getSize

CarbonDataQA · 2018-09-17T12:34:29Z

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

CarbonDataQA · 2018-09-17T13:37:58Z

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

CarbonDataQA · 2018-09-17T13:41:03Z

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

ajantha-bhat · 2018-09-18T04:39:34Z

@ravipesala : PR is ready. Please check.

ravipesala · 2018-09-18T12:03:05Z

@xuchuanyin This 2MB limit causing many issues in varchar and complex columns. We cannot let user to configure this internal limits. We should have a growable stream. Besides, we better remove this bytebuffer and set directly to unsafe.

kevinjmh · 2018-09-18T12:04:52Z

...sing/src/main/java/org/apache/carbondata/processing/store/CarbonFactDataHandlerColumnar.java

@@ -239,6 +239,7 @@ public void addDataToStore(CarbonRow row) throws CarbonDataWriterException {
   * @return false if any varchar column page cannot add one more value(2MB)
   */
  private boolean isVarcharColumnFull(CarbonRow row) {
+    //TODO: test and remove this as now  UnsafeSortDataRows can exceed 2MB


@ajantha-bhat Original implementation use 2MB to ensure next varchar column value can be filled safely, because size of value of single column won't exceed size of a row.
If UnsafeSortDataRows can exceed 2MB(growing dynamically), then we cannot check whether we have enough space for next value because we are not sure how many space next value will take. So the column page size check should be run before adding row to dataRows

I am not sure how we come to the conclusion of 2MB. There is no guarantee that we always sort the data to use UnsafeSortDataRows always. How about nosort case? And how about if user wants to add 100MB varchar how to support it.
And also this is not just limited to varchar, we should consider for complex and string columns as well here.
@ajantha-bhat Please remove that todo, But we need to refactor the code to ensure to keep the page size within the snappy max compressed length for complex and string datatypes as well.

@xuchuanyin @kevinjmh @ravipesala @kumarvishal09 : As per discussion let us handle this with configurable page size [from 1 MB to 2GB(snappy max)] and split the complex child pages here only and add validation for each column based on row,

This will be analyzed more and I will open a discussion in community and separate PR will be raised for this.

… columns that grows more than 2MB

CarbonDataQA · 2018-09-18T13:59:40Z

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

CarbonDataQA · 2018-09-18T14:57:05Z

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

CarbonDataQA · 2018-09-18T15:01:18Z

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

CarbonDataQA · 2018-09-18T15:09:37Z

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

ajantha-bhat · 2018-09-18T15:56:32Z

retest this please

CarbonDataQA · 2018-09-18T16:09:39Z

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

CarbonDataQA · 2018-09-18T17:11:48Z

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

CarbonDataQA · 2018-09-18T17:20:29Z

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

ajantha-bhat · 2018-09-19T05:11:07Z

@ravipesala : PR is ready please review

ravipesala · 2018-09-19T11:01:28Z

LGTM I am merging this PR. @ajantha-bhat Please start another discussion in the forum to support big column data up to 2GB for complex, varchar and string columns. And also make the page size configurable in terms of MB to avoid outofmemory while reading.

xuchuanyin · 2018-09-19T12:33:04Z

LGTM

xuchuanyin · 2018-09-19T12:33:51Z

please take care about the loading performance compared with previous nio.buffer implementation.

ajantha-bhat force-pushed the ensureArray branch from 761514a to 1e7bffc Compare September 11, 2018 16:05

ajantha-bhat changed the title ~~[WIP] multiple issue fixes for varchar column and complex columns, row that grows more than 2MB~~ [CARBONDATA-2927] multiple issue fixes for varchar column and complex columns, row that grows more than 2MB Sep 11, 2018

xuchuanyin reviewed Sep 12, 2018

View reviewed changes

ravipesala reviewed Sep 12, 2018

View reviewed changes

ajantha-bhat force-pushed the ensureArray branch from 1e7bffc to 66f8d2b Compare September 17, 2018 07:21

xuchuanyin reviewed Sep 17, 2018

View reviewed changes

ajantha-bhat force-pushed the ensureArray branch from 66f8d2b to 03c87a0 Compare September 17, 2018 10:17

ajantha-bhat force-pushed the ensureArray branch from 03c87a0 to bb133c9 Compare September 17, 2018 11:27

ravipesala reviewed Sep 17, 2018

View reviewed changes

ajantha-bhat force-pushed the ensureArray branch from bb133c9 to 8310516 Compare September 17, 2018 12:23

kevinjmh reviewed Sep 18, 2018

View reviewed changes

[CARBONDATA-2927] multiple issue fixes for varchar column and complex…

4eb0159

… columns that grows more than 2MB

reveiw comment fixes

48a91bc

ajantha-bhat force-pushed the ensureArray branch from 0fa9545 to 48a91bc Compare September 18, 2018 14:58

asfgit closed this in d1bfb74 Sep 19, 2018

[CARBONDATA-2927] multiple issue fixes for varchar column and complex columns, row that grows more than 2MB #2706

[CARBONDATA-2927] multiple issue fixes for varchar column and complex columns, row that grows more than 2MB #2706

Conversation

ajantha-bhat commented Sep 10, 2018 • edited

CarbonDataQA commented Sep 10, 2018

CarbonDataQA commented Sep 10, 2018

CarbonDataQA commented Sep 10, 2018

ajantha-bhat commented Sep 11, 2018

CarbonDataQA commented Sep 11, 2018

CarbonDataQA commented Sep 11, 2018

CarbonDataQA commented Sep 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravipesala Sep 12, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xuchuanyin commented Sep 12, 2018

Choose a reason for hiding this comment

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

Choose a reason for hiding this comment

ajantha-bhat Sep 18, 2018 • edited

Choose a reason for hiding this comment

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

CarbonDataQA commented Sep 17, 2018

ajantha-bhat commented Sep 18, 2018

ravipesala commented Sep 18, 2018

kevinjmh Sep 18, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CarbonDataQA commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

ajantha-bhat commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

CarbonDataQA commented Sep 18, 2018

ajantha-bhat commented Sep 19, 2018

ravipesala commented Sep 19, 2018

xuchuanyin commented Sep 19, 2018

xuchuanyin commented Sep 19, 2018

ajantha-bhat commented Sep 10, 2018 •

edited

ravipesala Sep 12, 2018 •

edited

ajantha-bhat Sep 18, 2018 •

edited

kevinjmh Sep 18, 2018 •

edited