You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/05/17 12:18:45 UTC
[GitHub] [arrow] lidavidm opened a new pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
lidavidm opened a new pull request #10344:
URL: https://github.com/apache/arrow/pull/10344
This backports the relevant part of ARROW-12500 into the 4.0.1 branch.
While ARROW-12500 cherry-picks cleanly, it doesn't build since it depends on prior changes - this just includes the actual fix and not the larger refactoring that was the focus of the patch.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842544869
CI failures:
- `Dev / Source Release and Merge Script`: unrelated
- `R / AMD64 Windows RTools 35 (pull_request)`: just warning
- `R / AMD64 Windows RTools 40 (pull_request)`: same warning
- `C GLib & Ruby / AMD64 MacOS 10.15 GLib & Ruby (pull_request)`: brew timeout
- `Python / AMD64 MacOS 10.15 Python 3 (pull_request)`: brew timeout
I'm going ahead and merge this.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#discussion_r633694586
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -112,6 +112,28 @@ N/A
ASSERT_EQ(row_count, 3);
}
+TEST_P(TestCsvFileFormat, ScanRecordBatchReaderSchema) {
+ // Regression test for ARROW-12603
+ auto source = GetFileSource(R"(f64,str
+1.0,2.0
+N/A,3.0
+2,foo)");
+ SetSchema({field("f64", float64()), field("str", utf8())});
+ ASSERT_OK_AND_ASSIGN(auto fragment, format_->MakeFragment(*source));
+ auto fragment_scan_options = std::make_shared<CsvFragmentScanOptions>();
+ // Force a small buffer size so type inference gets it wrong
+ fragment_scan_options->read_options.block_size = 20;
+ opts_->fragment_scan_options = fragment_scan_options;
+ ASSERT_OK(SetProjection(opts_.get(), {"f64"}));
+
+ int64_t row_count = 0;
+ for (auto maybe_batch : Batches(fragment.get())) {
+ ASSERT_OK_AND_ASSIGN(auto batch, maybe_batch);
+ row_count += batch->num_rows();
Review comment:
I assume it was failing previously since you don't check the columns.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842383305
Thanks @lidavidm for the backport! Could we cover this with a minimal test case?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on a change in pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs commented on a change in pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#discussion_r633694586
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -112,6 +112,28 @@ N/A
ASSERT_EQ(row_count, 3);
}
+TEST_P(TestCsvFileFormat, ScanRecordBatchReaderSchema) {
+ // Regression test for ARROW-12603
+ auto source = GetFileSource(R"(f64,str
+1.0,2.0
+N/A,3.0
+2,foo)");
+ SetSchema({field("f64", float64()), field("str", utf8())});
+ ASSERT_OK_AND_ASSIGN(auto fragment, format_->MakeFragment(*source));
+ auto fragment_scan_options = std::make_shared<CsvFragmentScanOptions>();
+ // Force a small buffer size so type inference gets it wrong
+ fragment_scan_options->read_options.block_size = 20;
+ opts_->fragment_scan_options = fragment_scan_options;
+ ASSERT_OK(SetProjection(opts_.get(), {"f64"}));
+
+ int64_t row_count = 0;
+ for (auto maybe_batch : Batches(fragment.get())) {
+ ASSERT_OK_AND_ASSIGN(auto batch, maybe_batch);
+ row_count += batch->num_rows();
Review comment:
I assume it was failing previously since you don't check the columns.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#discussion_r633695279
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -112,6 +112,28 @@ N/A
ASSERT_EQ(row_count, 3);
}
+TEST_P(TestCsvFileFormat, ScanRecordBatchReaderSchema) {
+ // Regression test for ARROW-12603
+ auto source = GetFileSource(R"(f64,str
+1.0,2.0
+N/A,3.0
+2,foo)");
+ SetSchema({field("f64", float64()), field("str", utf8())});
+ ASSERT_OK_AND_ASSIGN(auto fragment, format_->MakeFragment(*source));
+ auto fragment_scan_options = std::make_shared<CsvFragmentScanOptions>();
+ // Force a small buffer size so type inference gets it wrong
+ fragment_scan_options->read_options.block_size = 20;
+ opts_->fragment_scan_options = fragment_scan_options;
+ ASSERT_OK(SetProjection(opts_.get(), {"f64"}));
+
+ int64_t row_count = 0;
+ for (auto maybe_batch : Batches(fragment.get())) {
+ ASSERT_OK_AND_ASSIGN(auto batch, maybe_batch);
+ row_count += batch->num_rows();
Review comment:
Ah yes, without the patch, this will fail before it gets here because it will use the auto-inferred type and try to read the string column as a float64 column.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842278682
https://issues.apache.org/jira/browse/ARROW-12603
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842278682
https://issues.apache.org/jira/browse/ARROW-12603
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a change in pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on a change in pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#discussion_r633695279
##########
File path: cpp/src/arrow/dataset/file_csv_test.cc
##########
@@ -112,6 +112,28 @@ N/A
ASSERT_EQ(row_count, 3);
}
+TEST_P(TestCsvFileFormat, ScanRecordBatchReaderSchema) {
+ // Regression test for ARROW-12603
+ auto source = GetFileSource(R"(f64,str
+1.0,2.0
+N/A,3.0
+2,foo)");
+ SetSchema({field("f64", float64()), field("str", utf8())});
+ ASSERT_OK_AND_ASSIGN(auto fragment, format_->MakeFragment(*source));
+ auto fragment_scan_options = std::make_shared<CsvFragmentScanOptions>();
+ // Force a small buffer size so type inference gets it wrong
+ fragment_scan_options->read_options.block_size = 20;
+ opts_->fragment_scan_options = fragment_scan_options;
+ ASSERT_OK(SetProjection(opts_.get(), {"f64"}));
+
+ int64_t row_count = 0;
+ for (auto maybe_batch : Batches(fragment.get())) {
+ ASSERT_OK_AND_ASSIGN(auto batch, maybe_batch);
+ row_count += batch->num_rows();
Review comment:
Ah yes, without the patch, this will fail before it gets here because it will use the auto-inferred type and try to read the string column as a float64 column.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] nealrichardson commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
nealrichardson commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842548833
FTR that R warning will go away when you bump the versions to 4.0.1
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842474964
I've added a quick test case.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs merged pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs merged pull request #10344:
URL: https://github.com/apache/arrow/pull/10344
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
lidavidm commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842474964
I've added a quick test case.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kszucs commented on pull request #10344: ARROW-12603: [C++][Dataset] Backport fix for specifying CSV column types
Posted by GitBox <gi...@apache.org>.
kszucs commented on pull request #10344:
URL: https://github.com/apache/arrow/pull/10344#issuecomment-842383305
Thanks @lidavidm for the backport! Could we cover this with a minimal test case?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org