You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@orc.apache.org by "hinxx (via GitHub)" <gi...@apache.org> on 2023/03/29 20:14:39 UTC
[GitHub] [orc] hinxx opened a new pull request, #1455: ORC-1401: [C++] allow writing an intermediate footer
hinxx opened a new pull request, #1455:
URL: https://github.com/apache/orc/pull/1455
Current C++ code base does not support writing intermediate footers as Java code base does.
It would be beneficial to the C++ based writers to have an ability to write intermediate footer(s) to the opened file while streaming data into it. The benefit is not losing the so far persisted data (after flushing stripes) in case of writer failure in closing the file (application crash).
The writer that wants to support the feature would follow similar approach as Hive does in writing a side file with the latest footer offsets. The reader would be able to determine the location of the last valid footer by using the last footer location from the side file (instead of assuming that the footer is at the end of the file).
The change was tested by creating a small ORC file writer test application that calls newly added `WriterImpl::writeIntermediateFooter()` after adding a batch of rows:
```
writer->add(*batch);
writer->writeIntermediateFooter();
```
The resulting ORC file is readable by the provided stock C++ and Java tools. The preliminary C++ test code and results from the tools are attached to this PR here [notes.txt](https://github.com/apache/orc/files/11105096/notes.txt).
Not all aspects of the possible use were tested yet namely unclean writer app shutdown, writing of the side file, loading of the side file, reading of the last valid footer when file was not properly closed.
This is a draft PR. No proper tests are added at the moment because of incomplete PR.
I'm looking for opinions from the ORC experts in order to finish up the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1507817301
Some CI checks fail due to following reason, please use `static_cast` to fix those errors.
```
/Users/runner/work/orc/orc/c++/src/Writer.cc:324:10: error: virtual function 'writeIntermediateFooter' has a different return type ('long') than the function it overrides (which has return type 'int64_t' (aka 'long long'))
long writeIntermediateFooter() override;
~~~~ ^
/Users/runner/work/orc/orc/c++/include/orc/Writer.hh:312:21: note: overridden virtual function is here
virtual int64_t writeIntermediateFooter() = 0;
~~~~~~~ ^
/Users/runner/work/orc/orc/c++/src/Writer.cc:418:36: error: implicit conversion changes signedness: 'uint64_t' (aka 'unsigned long long') to 'long' [-Werror,-Wsign-conversion]
lastFlushOffset = outStream->getLength();
~ ~~~~~~~~~~~^~~~~~~~~~~
/Users/runner/work/orc/orc/c++/src/Writer.cc:419:23: error: implicit conversion changes signedness: 'long' to 'uint64_t' (aka 'unsigned long long') [-Werror,-Wsign-conversion]
currentOffset = lastFlushOffset;
~ ^~~~~~~~~~~~~~~
3 errors generated.
```
In addition, the C++ format check failures can be fixed by applying `clang-format`. @hinxx
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1163795558
##########
c++/src/Writer.cc:
##########
@@ -307,6 +307,8 @@ namespace orc {
static const char* magicId;
static const WriterId writerId;
bool useTightNumericVector;
+ long stripesAtLastFlush;
Review Comment:
If I use `size_t` my compiler complains:
```
error: comparison of integer expressions of different signedness: ‘size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
412 | if (stripesAtLastFlush != fileFooter.stripes_size()) {
| ~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
`Footer::stripes_size()` returns `int`, instead of casting to `size_t` maybe we could just do: `int stripesAtLastFlush;`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1162782666
##########
c++/src/Writer.cc:
##########
@@ -400,6 +405,24 @@ namespace orc {
outStream->close();
}
+ long WriterImpl::writeIntermediateFooter() {
+ if (stripeRows > 0) {
+ writeStripe();
+ }
+ if (stripesAtLastFlush != fileFooter.stripes_size()) {
+ writeMetadata();
+ writeFileFooter();
+ stripesAtLastFlush = fileFooter.stripes_size();
+ writePostscript();
Review Comment:
I guess so since `fileFooter` is not touched in `writePostscript()`. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1152440779
##########
c++/src/Writer.cc:
##########
@@ -400,6 +406,24 @@ namespace orc {
outStream->close();
}
+ long WriterImpl::writeIntermediateFooter() {
+ if (stripeRows > 0) {
+ writeStripe();
+ }
+ if (stripesAtLastFlush != numStripes) {
+ writeMetadata();
+ writeFileFooter();
+ stripesAtLastFlush = numStripes;
+ writePostscript();
+ outStream->flush();
+ lastFlushOffset = outStream->getLength();
+ currentOffset = lastFlushOffset;
+ // init stripe now that we adjusted the currentOffset
+ initStripe();
+ }
+ return stripesAtLastFlush;
Review Comment:
This is wrong, should be `return lastFlushOffset;`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1489260246
cc @omalley and @wgtmac
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1163835145
##########
c++/src/Writer.cc:
##########
@@ -307,6 +307,8 @@ namespace orc {
static const char* magicId;
static const WriterId writerId;
bool useTightNumericVector;
+ long stripesAtLastFlush;
Review Comment:
We can apply static_cast here. Something like `if (stripesAtLastFlush != static_cast<size_t>(fileFooter.stripes_size())) `. Or we can make it int32_t here because `fileFooter.stripes_size()` is of type `int`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1152437326
##########
c++/src/Writer.cc:
##########
@@ -307,6 +307,9 @@ namespace orc {
static const char* magicId;
static const WriterId writerId;
bool useTightNumericVector;
+ long numStripes;
Review Comment:
I guess numStripes could be replaced by the `fileFooter.stripes_size()`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac merged pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac merged PR #1455:
URL: https://github.com/apache/orc/pull/1455
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1152439631
##########
c++/src/Writer.cc:
##########
@@ -542,6 +568,7 @@ namespace orc {
// update file statistics
std::vector<proto::ColumnStatistics> colStats;
columnWriter->getFileStatistics(colStats);
+ fileFooter.clear_statistics();
Review Comment:
I'm not sure if this is the right place to do this. It seems to give desired result when doing the intermediate footer writes that cause stripe flush/write, but I'm thinking that it might not be OK when stripes are written as part of normal flushing due to their size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] dongjoon-hyun commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1522613004
Thank you, @hinxx and @wgtmac !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1512790464
I checked the CI logs and I think I solved all the issue regarding the data types and formatting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1513415314
> I checked the CI logs and I think I solved all the issue regarding the data types and formatting.
Seems the build on macos has a different opinion.
```
[ 58%] Building CXX object c++/src/CMakeFiles/orc.dir/Writer.cc.o
/Users/runner/work/orc/orc/c++/src/Writer.cc:419:36: error: implicit conversion changes signedness: 'uint64_t' (aka 'unsigned long long') to 'int64_t' (aka 'long long') [-Werror,-Wsign-conversion]
lastFlushOffset = outStream->getLength();
~ ~~~~~~~~~~~^~~~~~~~~~~
/Users/runner/work/orc/orc/c++/src/Writer.cc:420:23: error: implicit conversion changes signedness: 'int64_t' (aka 'long long') to 'uint64_t' (aka 'unsigned long long') [-Werror,-Wsign-conversion]
currentOffset = lastFlushOffset;
~ ^~~~~~~~~~~~~~~
/Users/runner/work/orc/orc/c++/src/Writer.cc:424:12: error: implicit conversion changes signedness: 'int64_t' (aka 'long long') to 'uint64_t' (aka 'unsigned long long') [-Werror,-Wsign-conversion]
return lastFlushOffset;
~~~~~~ ^~~~~~~~~~~~~~~
3 errors generated.
make[2]: *** [c++/src/CMakeFiles/orc.dir/Writer.cc.o] Error 1
make[1]: *** [c++/src/CMakeFiles/orc.dir/all] Error 2
make: *** [all] Error 2
Error: Process completed with exit code 2.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1152674596
##########
c++/include/orc/OrcFile.hh:
##########
@@ -98,6 +98,11 @@ namespace orc {
* Close the stream and flush any pending data to the disk.
*/
virtual void close() = 0;
+
+ /**
+ * Flush any pending data to the disk.
+ */
+ virtual void flush() = 0;
Review Comment:
What about providing a default implementation which throws an unimplemented exception? Otherwise I am afraid that it may break the downstream to upgrade smoothly.
##########
c++/include/orc/Writer.hh:
##########
@@ -303,6 +303,13 @@ namespace orc {
* Add user metadata to the writer.
*/
virtual void addUserMetadata(const std::string& name, const std::string& value) = 0;
+
+ /**
+ * Write an intermediate footer on the file such that if the file is
+ * truncated to the returned offset, it would be a valid ORC file.
+ * @return the offset that would be a valid end location for an ORC file
+ */
+ virtual long writeIntermediateFooter() = 0;
Review Comment:
Better to use fixed-length type as `long` semantic varies among platforms.
##########
c++/include/orc/Writer.hh:
##########
@@ -303,6 +303,13 @@ namespace orc {
* Add user metadata to the writer.
*/
virtual void addUserMetadata(const std::string& name, const std::string& value) = 0;
+
+ /**
+ * Write an intermediate footer on the file such that if the file is
+ * truncated to the returned offset, it would be a valid ORC file.
+ * @return the offset that would be a valid end location for an ORC file
+ */
+ virtual long writeIntermediateFooter() = 0;
Review Comment:
```suggestion
virtual int64_t writeIntermediateFooter() = 0;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on PR #1455:
URL: https://github.com/apache/orc/pull/1455#issuecomment-1514193703
> Seems the build on macos has a different opinion.
This is becoming embarrassing. I would expect to see these sign related errors in my Linux g++ build, but for some reason I don't.
Changed `lastFlushOffset` data type to `uint64_t`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1163491151
##########
c++/src/Writer.cc:
##########
@@ -307,6 +307,8 @@ namespace orc {
static const char* magicId;
static const WriterId writerId;
bool useTightNumericVector;
+ long stripesAtLastFlush;
Review Comment:
```suggestion
size_t stripesAtLastFlush;
```
##########
c++/src/Writer.cc:
##########
@@ -307,6 +307,8 @@ namespace orc {
static const char* magicId;
static const WriterId writerId;
bool useTightNumericVector;
+ long stripesAtLastFlush;
+ long lastFlushOffset;
Review Comment:
```suggestion
int64_t lastFlushOffset;
```
##########
c++/src/Writer.cc:
##########
@@ -319,6 +321,8 @@ namespace orc {
void addUserMetadata(const std::string& name, const std::string& value) override;
+ long writeIntermediateFooter() override;
Review Comment:
```suggestion
int64_t writeIntermediateFooter() override;
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] wgtmac commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1154052325
##########
c++/test/TestWriter.cc:
##########
@@ -1994,6 +2002,109 @@ namespace orc {
EXPECT_FALSE(rowReader->next(*batch));
}
+ TEST_P(WriterTest, writeIntFileFlush) {
+ MemoryOutputStream memStream(DEFAULT_MEM_STREAM_SIZE);
+ MemoryPool* pool = getDefaultPool();
+ std::unique_ptr<Type> type(Type::buildTypeFromString("struct<col1:int>"));
+
+ uint64_t stripeSize = 1024; // 1K
+ uint64_t compressionBlockSize = 1024; // 1k
+
+ std::unique_ptr<Writer> writer =
+ createWriter(stripeSize, compressionBlockSize, CompressionKind_ZLIB, *type, pool,
+ &memStream, fileVersion);
+ std::unique_ptr<ColumnVectorBatch> batch = writer->createRowBatch(65535);
+ StructVectorBatch* structBatch = dynamic_cast<StructVectorBatch*>(batch.get());
+ LongVectorBatch* longBatch = dynamic_cast<LongVectorBatch*>(structBatch->fields[0]);
+
+ for (uint64_t j = 0; j < 10; ++j) {
+ for (uint64_t i = 0; i < 65535; ++i) {
+ longBatch->data[i] = static_cast<int64_t>(i);
+ }
+ structBatch->numElements = 65535;
+ longBatch->numElements = 65535;
+
+ writer->add(*batch);
+ // force writing a stripe
+ writer->writeIntermediateFooter();
Review Comment:
It would be better to save all 10 locations to intermediate footers and open them respectively to verify only partial data can be read correctly.
##########
c++/src/Writer.cc:
##########
@@ -400,6 +405,24 @@ namespace orc {
outStream->close();
}
+ long WriterImpl::writeIntermediateFooter() {
+ if (stripeRows > 0) {
+ writeStripe();
+ }
+ if (stripesAtLastFlush != fileFooter.stripes_size()) {
+ writeMetadata();
+ writeFileFooter();
+ stripesAtLastFlush = fileFooter.stripes_size();
+ writePostscript();
Review Comment:
Is it safe to reorder these two lines?
##########
c++/src/Writer.cc:
##########
@@ -542,6 +568,7 @@ namespace orc {
// update file statistics
std::vector<proto::ColumnStatistics> colStats;
columnWriter->getFileStatistics(colStats);
+ fileFooter.clear_statistics();
Review Comment:
This should be OK, as we have anyway to overwrite file-level stats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [orc] hinxx commented on a diff in pull request #1455: ORC-1401: [C++] allow writing an intermediate footer
Posted by "hinxx (via GitHub)" <gi...@apache.org>.
hinxx commented on code in PR #1455:
URL: https://github.com/apache/orc/pull/1455#discussion_r1162783237
##########
c++/test/TestWriter.cc:
##########
@@ -1994,6 +2002,109 @@ namespace orc {
EXPECT_FALSE(rowReader->next(*batch));
}
+ TEST_P(WriterTest, writeIntFileFlush) {
+ MemoryOutputStream memStream(DEFAULT_MEM_STREAM_SIZE);
+ MemoryPool* pool = getDefaultPool();
+ std::unique_ptr<Type> type(Type::buildTypeFromString("struct<col1:int>"));
+
+ uint64_t stripeSize = 1024; // 1K
+ uint64_t compressionBlockSize = 1024; // 1k
+
+ std::unique_ptr<Writer> writer =
+ createWriter(stripeSize, compressionBlockSize, CompressionKind_ZLIB, *type, pool,
+ &memStream, fileVersion);
+ std::unique_ptr<ColumnVectorBatch> batch = writer->createRowBatch(65535);
+ StructVectorBatch* structBatch = dynamic_cast<StructVectorBatch*>(batch.get());
+ LongVectorBatch* longBatch = dynamic_cast<LongVectorBatch*>(structBatch->fields[0]);
+
+ for (uint64_t j = 0; j < 10; ++j) {
+ for (uint64_t i = 0; i < 65535; ++i) {
+ longBatch->data[i] = static_cast<int64_t>(i);
+ }
+ structBatch->numElements = 65535;
+ longBatch->numElements = 65535;
+
+ writer->add(*batch);
+ // force writing a stripe
+ writer->writeIntermediateFooter();
Review Comment:
Thanks for the suggestion; done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@orc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org