TpchBenchmark is broken

[majetideepak]

Bug description

It currently segfaults on cmake-build-debug/velox/benchmarks/tpch/velox_tpch_benchmark --data_path data/hive_data/tpch --run_query_verbose 6
Specifying the cache_gb option throws the following message
MemoryAllocator capacity 10737418240 must be the same as MemoryManager capacity 9223372036854775807.

The following patch is needed to prevent the segfault.

--- a/velox/benchmarks/tpch/TpchBenchmark.cpp
+++ b/velox/benchmarks/tpch/TpchBenchmark.cpp
@@ -263,7 +263,9 @@ class TpchBenchmark {
   }
 
   void shutdown() {
-    cache_->shutdown();
+    if (cache_) {
+      cache_->shutdown();
+    }
   }
 
   std::pair<std::unique_ptr<TaskCursor>, std::vector<RowVectorPtr>> run(

System information

Velox System Info v0.0.2
Commit: ebf797a
CMake Version: 3.26.4
System: Darwin-22.6.0
Arch: arm64
C++ Compiler: /Library/Developer/CommandLineTools/usr/bin/c++
C++ Compiler Version: 15.0.0.15000040
C Compiler: /Library/Developer/CommandLineTools/usr/bin/cc
C Compiler Version: 15.0.0.15000040
CMake Prefix Path: /Library/Developer/CommandLineTools/SDKs/MacOSX14.0.sdk/usr;/opt/homebrew;/usr/local;/usr;/;/opt/homebrew/Cellar/cmake/3.26.4;/usr/local;/usr/X11R6;/usr/pkg;/opt;/sw;/opt/local

Relevant logs

No response

[XinShuoWang]

Same issue. Why is this fix not merged into the main branch?

[majetideepak]

@XinShuoWang Along with the fix, we need to add this benchmark to CI to prevent this from breaking again.

[bdice]

It seems like PR #7345 went stale. Could this fix be revived and merged as-is? We have confirmed that it fixes a bug on our side.

[majetideepak]

This seems to be fixed already here https://github.com/facebookincubator/velox/pull/10679/files#diff-dbd700812476d0ed9d8e26a408cf2a2a6c62cd3b8f87881d770bde5fe2b73948R210

[majetideepak]

@bdice can you share a reproducer?

[bdice]

Ah - I just saw this issue was still open. I have a reproducer but it may be because I may need to pull in the latest Velox upstream (my branch is out of date). I'll double check on the upstream soon, but I'm fine with closing this issue if you think it has been resolved.

cc: @karthikeyann

[bdice]

I verified that the fix linked above worked. I think this issue should be safe to close now.

[karthikeyann]

It's broken again after new parquet reader
This PR #11833 fixes the issue.

[majetideepak]

Currently fixed!