zanmato1984 commented on code in PR #43698:
URL: https://github.com/apache/arrow/pull/43698#discussion_r2334395895
##########
cpp/src/arrow/util/thread_pool.h:
##########
@@ -19,6 +19,7 @@
#include <cstdint>
#include <memory>
+#include <optional>
Review Comment:
```suggestion
```
##########
cpp/src/arrow/dataset/scanner.h:
##########
@@ -104,6 +105,11 @@ struct ARROW_DS_EXPORT ScanOptions {
/// Note: The IOContext executor will be ignored if use_threads is set to
false
io::IOContext io_context;
+ /// Executor for any CPU tasks
Review Comment:
Maybe give a description about the semantic implied by executor being null?
##########
cpp/src/arrow/util/thread_pool.h:
##########
@@ -19,6 +19,7 @@
#include <cstdint>
#include <memory>
+#include <optional>
Review Comment:
Not needed?
##########
cpp/src/arrow/dataset/file_parquet_test.cc:
##########
@@ -973,5 +973,52 @@ TEST_F(TestParquetFileFormat, MultithreadedScanRegression)
{
}
}
+TEST_F(TestParquetFileFormat, MultithreadedComputeRegression) {
Review Comment:
This test is mostly identical as its customized IO context counterpart. Can
we abstract the most common part of the tests, i.e., the logic except
customizing the `ScanOptions`, and make different individual tests by passing
in different `ScanOptions`s?
##########
cpp/src/arrow/dataset/scanner.h:
##########
@@ -104,6 +105,11 @@ struct ARROW_DS_EXPORT ScanOptions {
/// Note: The IOContext executor will be ignored if use_threads is set to
false
io::IOContext io_context;
+ /// Executor for any CPU tasks
+ ///
+ /// Note: The Executor will be ignored if use_threads is set to false
+ arrow::internal::Executor* cpu_executor = nullptr;
Review Comment:
Hi @kou , do we still require `NULLPTR` in header files?
##########
cpp/src/arrow/dataset/scanner.h:
##########
@@ -104,6 +105,11 @@ struct ARROW_DS_EXPORT ScanOptions {
/// Note: The IOContext executor will be ignored if use_threads is set to
false
io::IOContext io_context;
+ /// Executor for any CPU tasks
+ ///
+ /// Note: The Executor will be ignored if use_threads is set to false
+ arrow::internal::Executor* cpu_executor = nullptr;
Review Comment:
```suggestion
arrow::internal::Executor* cpu_executor = NULLPTR;
```
##########
cpp/src/arrow/dataset/file_parquet_test.cc:
##########
@@ -973,5 +973,52 @@ TEST_F(TestParquetFileFormat, MultithreadedScanRegression)
{
}
}
+TEST_F(TestParquetFileFormat, MultithreadedComputeRegression) {
+ // GH-43694: Test similar situation as MultithreadedScanRegression but with
+ // the exec context instead
Review Comment:
```suggestion
// customized CPU executor instead
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
