ORC-1312: [C++] Support setting the block buffer capacity of BufferedOutputStream #1394

coderex2522 · 2023-02-01T04:16:06Z

What changes were proposed in this pull request?

This pr provides the API to set the initial buffer capacity of BufferedOutputStream.

Why are the changes needed?

It's convenient for users to adjust the buffer size of the BufferedOutputStream according to the written data.

How was this patch tested?

Add the WriterTest.setOutputBufferCapacity to test different buffer capacity of BufferedOutputStream.
In addition, I did a simple test similar to this issue

PeakMem	BufferCapacity = 1MB	BufferCapacity = 64KB
10000 Fields	38.96GB	11.48GB
1000 Fields	3.92GB	1.17GB
100 Fields	0.41GB	0.13GB

…Stream

dongjoon-hyun · 2023-02-01T04:17:22Z

c++/include/orc/Writer.hh

@@ -262,6 +262,17 @@ namespace orc {
     * @return if not set, the default is false
     */
    bool getUseTightNumericVector() const;
+
+     /**


nit. Indentation?

This is detected by the CI format check.

c++/include/orc/Writer.hh

dongjoon-hyun · 2023-02-01T04:21:02Z

c++/test/TestWriter.cc

+    }
+  }
+
+  TEST(WriterTest, setOutputBufferCapacity) {


Thank you for adding this.

dongjoon-hyun · 2023-02-01T04:21:33Z

c++/test/TestWriter.cc

+  TEST(WriterTest, setOutputBufferCapacity) {
+    testSetOutputBufferCapacity(1024);
+    testSetOutputBufferCapacity(1024 * 1024);
+    testSetOutputBufferCapacity(1024 * 1024 * 1024);


If this is 1GB, I'd remove this from unit test.

The 1GB case has been deleted.

dongjoon-hyun · 2023-02-01T04:22:22Z

cc @wgtmac , @stiga-huang , @williamhyun , @guiyanakuang

dongjoon-hyun

Please check and fix Windows failure.

unknown file: error: C++ exception with description "bad allocation" thrown in the test body.
[  FAILED  ] WriterTest.setOutputBufferCapacity (44714 ms)

wgtmac · 2023-02-01T05:10:30Z

c++/include/orc/Writer.hh

@@ -262,6 +262,17 @@ namespace orc {
     * @return if not set, the default is false
     */
    bool getUseTightNumericVector() const;
+
+     /**


This is detected by the CI format check.

c++/include/orc/Writer.hh

wgtmac · 2023-02-01T05:14:32Z

c++/test/TestWriter.cc

+  TEST(WriterTest, setOutputBufferCapacity) {
+    testSetOutputBufferCapacity(1024);
+    testSetOutputBufferCapacity(1024 * 1024);
+    testSetOutputBufferCapacity(1024 * 1024 * 1024);


wgtmac · 2023-02-01T05:18:29Z

c++/test/TestWriter.cc

+        Type::buildTypeFromString("struct<col1:int,col2:int>"));
+      WriterOptions options;
+      options.setStripeSize(1024 * 1024)
+          .setCompressionBlockSize(1024)


Should we change setCompressionBlockSize accordingly? It seems that even you have changed the output buffer size but the page is very small and no resize will happen actually.

wgtmac · 2023-02-02T16:00:17Z

c++/include/orc/Writer.hh

@@ -262,6 +262,19 @@ namespace orc {
     * @return if not set, the default is false
     */
    bool getUseTightNumericVector() const;
+
+    /**
+     * Set the initial buffer capacity of output stream.


Suggested change

* Set the initial buffer capacity of output stream.

* Set the initial capacity to the output buffer of the compressed stream.

wgtmac · 2023-02-02T16:02:57Z

c++/include/orc/Writer.hh

+
+    /**
+     * Set the initial buffer capacity of output stream.
+     * Each column contains multiple output streams with buffers,


Suggested change

* Each column contains multiple output streams with buffers,

* Each column contains one or more compressed streams depending on its type,

Class CompressionStream is a subclass of class BufferedOutputStream which contains the buffer. And BufferedOutputStream has no logic associated with the compression algorithm, so it may not be appropriate to use compressed stream to describe it.

Then it makes sense to me to use BufferedOutputStream explicitly. We have defined several output streams (e.g. one from OrcFile.hh). It should be clear to user which one we are referring to.

wgtmac · 2023-02-02T16:03:34Z

c++/include/orc/Writer.hh

+    /**
+     * Set the initial buffer capacity of output stream.
+     * Each column contains multiple output streams with buffers,
+     * and these buffers will automatically expand when memory is exhausted.


Suggested change

* and these buffers will automatically expand when memory is exhausted.

* and these buffers will automatically expand when more memory is required.

Correct me if I am wrong here.

When the used size of the buffer reaches the initial capacity, the buffer will continue to expand in compression block size units.

In the CompressionStream, will the output buffer exceed the compression block size?

The outputBuffer(char* variable) of the class CompressionStreamBase doesn't exceed the compression block size, and the dataBuffer(BlockBuffer variable) of the class BufferedOutputStream will exceed the compression block size.

wgtmac · 2023-02-02T16:04:12Z

c++/include/orc/Writer.hh

+    WriterOptions& setOutputBufferCapacity(uint64_t capacity);
+
+    /**
+     * Get the buffer capacity of output stream.


Suggested change

* Get the buffer capacity of output stream.

* Get the initial capacity of output buffer in the class BufferedOutputStream.

wgtmac

Thanks for the explanation. My point is to explicitly describe which buffer applies the new options. It is kind of vague to me by simply saying output buffer and output stream.

wgtmac · 2023-02-03T05:22:35Z

c++/include/orc/Writer.hh

+
+    /**
+     * Set the initial buffer capacity of output stream.
+     * Each column contains multiple output streams with buffers,


Then it makes sense to me to use BufferedOutputStream explicitly. We have defined several output streams (e.g. one from OrcFile.hh). It should be clear to user which one we are referring to.

wgtmac · 2023-02-03T05:25:37Z

c++/include/orc/Writer.hh

+    /**
+     * Set the initial buffer capacity of output stream.
+     * Each column contains multiple output streams with buffers,
+     * and these buffers will automatically expand when memory is exhausted.


In the CompressionStream, will the output buffer exceed the compression block size?

dongjoon-hyun · 2023-02-03T07:45:58Z

+1 for @wgtmac 's advice.

BTW, googletest failures in the CI is related to this PR?

CMake Error at googletest_ep-stamp/googletest_ep-download-RELWITHDEBINFO.cmake:49 (message):
  Command failed: 1
   '/usr/local/bin/cmake' '-Dmake=' '-Dconfig=' '-P' '/home/runner/work/orc/orc/build/googletest_ep-prefix/src/googletest_ep-stamp/googletest_ep-download-RELWITHDEBINFO-impl.cmake'

coderex2522 · 2023-02-03T14:29:36Z

It seems to be caused by googletest package download failure. Compile successfully after retry.

wgtmac

Thanks! LGTM

dongjoon-hyun

+1, LGTM. Thank you, @coderex2522 and @wgtmac .

wgtmac · 2023-02-04T04:03:27Z

@coderex2522 I will leave it to you to merge it to test your permission.

dongjoon-hyun · 2023-02-07T15:57:27Z

Congratulation, @coderex2522 . :)

dongjoon-hyun · 2023-02-07T15:59:06Z

FYI, we provide merge script like Apache Spark community which preserves the committer identities when you merge someone else's PR.

https://github.com/apache/orc/blob/main/dev/merge_orc_pr.py

williamhyun · 2023-02-11T05:14:20Z

@coderex2522

Hey Xin, I've made an official announcement in the ORC dev and user mailing list welcoming you as our new committer.
If you have not yet subscribed to these mailing lists, please do so ASAP.

In addition, Could you make a PR to update the ORC website to add you as a committer?

https://orc.apache.org/develop/committers/
This is the page that should be updated.

coderex2522 · 2023-02-11T16:02:55Z

In addition, Could you make a PR to update the ORC website to add you as a committer?

Thanks, I've subscribed to these mailing lists before. And I will submit a PR to update the ORC website as soon as possible.

…OutputStream (apache#1394) * ORC-1312: [C++] Support setting the capacity of output buffer in the class BufferedOutputStream

coderex2522 added 4 commits November 29, 2022 22:36

ORC-1312: [C++] Support setting the buffer capacity of BufferedOutput…

73aaa7a

…Stream

ORC-1312: [C++] add the ut for setting output buffer capacity

98718c9

Merge branch 'apache:main' into ORC-1312

12930b9

Merge branch 'apache:main' into ORC-1312

aa18dd5

github-actions bot added the CPP label Feb 1, 2023

dongjoon-hyun reviewed Feb 1, 2023

View reviewed changes

c++/include/orc/Writer.hh Show resolved Hide resolved

dongjoon-hyun reviewed Feb 1, 2023

View reviewed changes

c++/test/TestWriter.cc

}

}

TEST(WriterTest, setOutputBufferCapacity) {

Copy link

Member

dongjoon-hyun Feb 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for adding this.

dongjoon-hyun reviewed Feb 1, 2023

View reviewed changes

wgtmac reviewed Feb 1, 2023

View reviewed changes

fix the format error and remove the 1GB test

178aec1

wgtmac reviewed Feb 2, 2023

View reviewed changes

wgtmac reviewed Feb 3, 2023

View reviewed changes

fix the description of the setOutputBufferCapacity api

5ce66fe

wgtmac approved these changes Feb 3, 2023

View reviewed changes

dongjoon-hyun approved these changes Feb 4, 2023

View reviewed changes

coderex2522 merged commit 32dffd3 into apache:main Feb 7, 2023

cxzl25 pushed a commit to cxzl25/orc that referenced this pull request Jan 11, 2024

ORC-1312: [C++] Support setting the block buffer capacity of Buffered…

0e3ff2b

…OutputStream (apache#1394) * ORC-1312: [C++] Support setting the capacity of output buffer in the class BufferedOutputStream

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ORC-1312: [C++] Support setting the block buffer capacity of BufferedOutputStream #1394

ORC-1312: [C++] Support setting the block buffer capacity of BufferedOutputStream #1394

coderex2522 commented Feb 1, 2023

dongjoon-hyun Feb 1, 2023

wgtmac Feb 1, 2023

coderex2522 Feb 2, 2023

dongjoon-hyun Feb 1, 2023

dongjoon-hyun Feb 1, 2023

wgtmac Feb 1, 2023

coderex2522 Feb 2, 2023

dongjoon-hyun commented Feb 1, 2023

dongjoon-hyun left a comment •

edited

Loading

wgtmac Feb 1, 2023

wgtmac Feb 1, 2023

wgtmac Feb 1, 2023

wgtmac Feb 2, 2023

wgtmac Feb 2, 2023

coderex2522 Feb 3, 2023

wgtmac Feb 3, 2023

wgtmac Feb 2, 2023 •

edited

Loading

wgtmac Feb 2, 2023

coderex2522 Feb 3, 2023

wgtmac Feb 3, 2023

coderex2522 Feb 3, 2023

wgtmac Feb 2, 2023 •

edited

Loading

wgtmac left a comment

wgtmac Feb 3, 2023

wgtmac Feb 3, 2023

dongjoon-hyun commented Feb 3, 2023

coderex2522 commented Feb 3, 2023

wgtmac left a comment

dongjoon-hyun left a comment

wgtmac commented Feb 4, 2023

dongjoon-hyun commented Feb 7, 2023

dongjoon-hyun commented Feb 7, 2023

williamhyun commented Feb 11, 2023

coderex2522 commented Feb 11, 2023

	* Set the initial buffer capacity of output stream.
	* Set the initial capacity to the output buffer of the compressed stream.

	* Each column contains multiple output streams with buffers,
	* Each column contains one or more compressed streams depending on its type,

	* and these buffers will automatically expand when memory is exhausted.
	* and these buffers will automatically expand when more memory is required.

	* Get the buffer capacity of output stream.
	* Get the initial capacity of output buffer in the class BufferedOutputStream.

ORC-1312: [C++] Support setting the block buffer capacity of BufferedOutputStream #1394

ORC-1312: [C++] Support setting the block buffer capacity of BufferedOutputStream #1394

Conversation

coderex2522 commented Feb 1, 2023

What changes were proposed in this pull request?

Why are the changes needed?

How was this patch tested?

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

dongjoon-hyun commented Feb 1, 2023

dongjoon-hyun left a comment • edited Loading

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

Choose a reason for hiding this comment

wgtmac Feb 2, 2023 • edited Loading

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

wgtmac Feb 2, 2023 • edited Loading

Choose a reason for hiding this comment

wgtmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Feb 3, 2023

coderex2522 commented Feb 3, 2023

wgtmac left a comment

Choose a reason for hiding this comment

dongjoon-hyun left a comment

Choose a reason for hiding this comment

wgtmac commented Feb 4, 2023

dongjoon-hyun commented Feb 7, 2023

dongjoon-hyun commented Feb 7, 2023

williamhyun commented Feb 11, 2023

coderex2522 commented Feb 11, 2023

dongjoon-hyun left a comment •

edited

Loading

wgtmac Feb 2, 2023 •

edited

Loading

wgtmac Feb 2, 2023 •

edited

Loading