Skip to content

ARROW-6964: [C++][Dataset] Add multithread support to Scanner::ToTable#5721

Closed
fsaintjacques wants to merge 1 commit into
apache:masterfrom
fsaintjacques:ARROW-6964-parallel-scanner-to-table
Closed

ARROW-6964: [C++][Dataset] Add multithread support to Scanner::ToTable#5721
fsaintjacques wants to merge 1 commit into
apache:masterfrom
fsaintjacques:ARROW-6964-parallel-scanner-to-table

Conversation

@fsaintjacques

@fsaintjacques fsaintjacques commented Oct 23, 2019

Copy link
Copy Markdown
Contributor

The caller may request a parallel construction of the table. Scanner was refactored to own the ScanOptions and ScanContext members.

Comment thread cpp/src/arrow/dataset/scanner.cc Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer the explicit loop than nested Visitor.

Comment thread cpp/src/arrow/dataset/scanner.h Outdated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's time to expose a common ResourceContext class that has a MemoryPool and a ThreadPool?

@github-actions

Copy link
Copy Markdown

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just some comments.

Comment thread cpp/src/arrow/dataset/scanner.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think it's useful to make this inline.

Comment thread cpp/src/arrow/dataset/scanner.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner.h Outdated
Comment thread cpp/src/arrow/dataset/scanner.cc Outdated

@bkietz bkietz left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A few small comments

Comment thread cpp/src/arrow/dataset/scanner_test.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner_test.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner.h Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe this should be a task group instead of a thread pool. Then users can pass a serial task group to signal single threaded operation

Comment thread cpp/src/arrow/dataset/scanner.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
batches.emplace_back(batch);
batches.emplace_back(std::move(batch));

Comment thread cpp/src/arrow/dataset/scanner.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner.cc Outdated
Comment thread cpp/src/arrow/dataset/scanner.cc Outdated
@fsaintjacques fsaintjacques force-pushed the ARROW-6964-parallel-scanner-to-table branch from 8d6e72e to 10ed013 Compare October 24, 2019 16:02
The caller may request a parallel construction of the table. Scanner was
refactor to own the ScanOptions and ScanContext members.

The `use_threads` options was added to ScanOptions so the caller can
indicate if Scanner is allowed to use parallelism.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants