You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by majetideepak <gi...@git.apache.org> on 2018/06/03 17:09:48 UTC
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
GitHub user majetideepak opened a pull request:
https://github.com/apache/orc/pull/277
ORC-372: Enable valgrind for C++ travis-ci tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/majetideepak/orc ORC-372
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/orc/pull/277.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #277
----
commit 621f6467ace049c90b3044e67c274f9d276b3a0d
Author: Deepak Majeti <de...@...>
Date: 2018-06-03T16:52:39Z
ORC-372: Enable valgrind for C++ travis-ci tests
----
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by majetideepak <gi...@git.apache.org>.
Github user majetideepak commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r193159979
--- Diff: c++/src/Compression.cc ---
@@ -199,13 +199,16 @@ namespace orc {
uint64_t blockSize,
MemoryPool& pool);
+ ~ZlibCompressionStream() { end(); }
--- End diff --
Thanks!
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by wgtmac <gi...@git.apache.org>.
Github user wgtmac commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r192910566
--- Diff: c++/src/Compression.cc ---
@@ -282,6 +285,11 @@ DIAGNOSTIC_PUSH
}
}
+ void ZlibCompressionStream::end() {
+ (void)deflateEnd(&strm);
+ }
+
+
--- End diff --
remove this line
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by majetideepak <gi...@git.apache.org>.
Github user majetideepak commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r192919379
--- Diff: c++/src/TypeImpl.cc ---
@@ -258,31 +258,34 @@ namespace orc {
case STRUCT: {
StructVectorBatch *result = new StructVectorBatch(capacity, memoryPool);
+ std::unique_ptr<ColumnVectorBatch> return_value = std::unique_ptr<ColumnVectorBatch>(result);
--- End diff --
we need `result` of type `StructVectorBatch` to access fields on line 263
---
[GitHub] orc issue #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by majetideepak <gi...@git.apache.org>.
Github user majetideepak commented on the issue:
https://github.com/apache/orc/pull/277
There are indeed valgrind failures. I will push a followup patch to fix these.
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r193106126
--- Diff: c++/test/CMakeLists.txt ---
@@ -64,7 +64,12 @@ target_link_libraries (create-test-files
protobuf
)
-add_test (NAME orc-test COMMAND orc-test)
+if (TEST_VALGRIND_MEMCHECK)
+ add_test (orc-test
--- End diff --
You have trailing spaces here.
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/orc/pull/277
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by wgtmac <gi...@git.apache.org>.
Github user wgtmac commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r192909966
--- Diff: c++/src/TypeImpl.cc ---
@@ -498,54 +506,54 @@ namespace orc {
}
ORC_UNIQUE_PTR<Type> Type::buildTypeFromString(const std::string& input) {
- std::vector<std::pair<std::string, Type*> > res =
+ std::vector<std::pair<std::string, ORC_UNIQUE_PTR<Type> > > res =
TypeImpl::parseType(input, 0, input.size());
if (res.size() != 1) {
throw std::logic_error("Invalid type string.");
}
- return ORC_UNIQUE_PTR<Type>(res[0].second);
+ return std::move(res[0].second);
}
Type* TypeImpl::parseArrayType(const std::string &input,
--- End diff --
is it better to change return type to unique_ptr as well to be consistent? same for below.
---
[GitHub] orc issue #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by majetideepak <gi...@git.apache.org>.
Github user majetideepak commented on the issue:
https://github.com/apache/orc/pull/277
@wgtmac, @xndai can you take a look at this patch? ZLIB compression code seems to be leaking memory. Thanks!
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r193100003
--- Diff: c++/src/Compression.cc ---
@@ -199,13 +199,16 @@ namespace orc {
uint64_t blockSize,
MemoryPool& pool);
+ ~ZlibCompressionStream() { end(); }
--- End diff --
You need an override on this or the mac complains.
---
[GitHub] orc pull request #277: ORC-372: Enable valgrind for C++ travis-ci tests
Posted by wgtmac <gi...@git.apache.org>.
Github user wgtmac commented on a diff in the pull request:
https://github.com/apache/orc/pull/277#discussion_r192907281
--- Diff: c++/src/TypeImpl.cc ---
@@ -258,31 +258,34 @@ namespace orc {
case STRUCT: {
StructVectorBatch *result = new StructVectorBatch(capacity, memoryPool);
+ std::unique_ptr<ColumnVectorBatch> return_value = std::unique_ptr<ColumnVectorBatch>(result);
--- End diff --
maybe we can directly merge line 260 and 261 to a single statement. same for below.
---