Skip to content

Implement skip_rows + column_names in R - #4

Merged
pitrou merged 2 commits into
pitrou:ARROW-5747-csv-column-namesfrom
nealrichardson:csv-reader
Jul 23, 2019
Merged

Implement skip_rows + column_names in R#4
pitrou merged 2 commits into
pitrou:ARROW-5747-csv-column-namesfrom
nealrichardson:csv-reader

Conversation

@nealrichardson

Copy link
Copy Markdown

The new behavior is working as expected now. Two other observations:

  1. The error message(s) are not great when you give bad input. For example, if I give too many or too few column_names, the error I get is Invalid: Empty CSV file, same as what I was getting before your latest change to the skip_rows behavior. It would be nice if the error message were accurate, and even better if it were more specific so that I as a user might know how to fix my bad input.
  2. On a related note, if ignore_empty_lines is false and there are empty lines, it fails to parse (again, with Invalid: Empty CSV file).

Happy to take those up in a separate ticket if you prefer, just observed them while working on this.

@pitrou

Copy link
Copy Markdown
Owner

A separate ticket sounds fine.

@nealrichardson

Copy link
Copy Markdown
Author

@codecov

codecovBot commented Jul 22, 2019

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (ARROW-5747-csv-column-names@f7c406f). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## ARROW-5747-csv-column-names #4 +/- ##
==============================================================
Coverage ? 75.27% ==============================================================
Files ? 57 Lines ? 3713 Branches ? 0 ==============================================================
Hits ? 2795 Misses ? 918 Partials ? 0
Impacted FilesCoverage Δ
r/src/csv.cpp100% <100%> (ø)
r/R/csv.R97.8% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7c406f...2786c60. Read the comment docs.

1 similar comment
@codecov

codecovBot commented Jul 22, 2019

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (ARROW-5747-csv-column-names@f7c406f). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@## ARROW-5747-csv-column-names #4 +/- ##
==============================================================
Coverage ? 75.27% ==============================================================
Files ? 57 Lines ? 3713 Branches ? 0 ==============================================================
Hits ? 2795 Misses ? 918 Partials ? 0
Impacted FilesCoverage Δ
r/src/csv.cpp100% <100%> (ø)
r/R/csv.R97.8% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7c406f...2786c60. Read the comment docs.

@pitrou
pitrou merged this pull request into pitrou:ARROW-5747-csv-column-namesJul 23, 2019
pitrou pushed a commit that referenced this pull request Feb 24, 2020
…comments.
The reset method allow the data structures to be re-used so they don't have to be allocated over and over again.
Closesapache#6430 from richardartoul/ra/merge-upstream and squashes the following commits:
5a08281 <Richard Artoul> Add license to test file
d76be05 <Richard Artoul> Add test for data reset
d102b1f <Richard Artoul> Add tests
d3e6e67 <Richard Artoul> cleanup comments
c8525ae <Richard Artoul> Add Reset method to int array (#5)
489ca25 <Richard Artoul> Fix array.setData() to retain before release (#4)
88cd05f <Richard Artoul> Add reset method to Data (#3)
6d1b277 <Richard Artoul> Add Reset() method to String array (#2)
dca2303 <Richard Artoul> Add Reset method to buffer and cleanup comments (#1)
Lead-authored-by: Richard Artoul <richard.artoul@datadoghq.com>
Co-authored-by: Richard Artoul <richardartoul@gmail.com>
Signed-off-by: Sebastien Binet <binet@cern.ch>
pitrou pushed a commit that referenced this pull request May 12, 2020
This PR enables tests for `ARROW_COMPUTE`, `ARROW_DATASET`, `ARROW_FILESYSTEM`, `ARROW_HDFS`, `ARROW_ORC`, and `ARROW_IPC` (default on). apache#7131 enabled a minimal set of tests as a starting point.
I confirmed that these tests pass locally with the current master. In the current TravisCI environment, we cannot see this result due to a lot of error messages in `arrow-utility-test`.
```
$ git log | head -1
commit ed5f534
% ctest
...
Start 1: arrow-array-test
1/51 Test #1: arrow-array-test ..................... Passed 4.62 sec
Start 2: arrow-buffer-test
2/51 Test #2: arrow-buffer-test .................... Passed 0.14 sec
Start 3: arrow-extension-type-test
3/51 Test #3: arrow-extension-type-test ............ Passed 0.12 sec
Start 4: arrow-misc-test
4/51 Test #4: arrow-misc-test ...................... Passed 0.14 sec
Start 5: arrow-public-api-test
5/51 Test #5: arrow-public-api-test ................ Passed 0.12 sec
Start 6: arrow-scalar-test
6/51 Test #6: arrow-scalar-test .................... Passed 0.13 sec
Start 7: arrow-type-test
7/51 Test #7: arrow-type-test ...................... Passed 0.14 sec
Start 8: arrow-table-test
8/51 Test #8: arrow-table-test ..................... Passed 0.13 sec
Start 9: arrow-tensor-test
9/51 Test #9: arrow-tensor-test .................... Passed 0.13 sec
Start 10: arrow-sparse-tensor-test
10/51 Test #10: arrow-sparse-tensor-test ............. Passed 0.16 sec
Start 11: arrow-stl-test
11/51 Test #11: arrow-stl-test ....................... Passed 0.12 sec
Start 12: arrow-concatenate-test
12/51 Test #12: arrow-concatenate-test ............... Passed 0.53 sec
Start 13: arrow-diff-test
13/51 Test #13: arrow-diff-test ...................... Passed 1.45 sec
Start 14: arrow-c-bridge-test
14/51 Test apache#14: arrow-c-bridge-test .................. Passed 0.18 sec
Start 15: arrow-io-buffered-test
15/51 Test apache#15: arrow-io-buffered-test ............... Passed 0.20 sec
Start 16: arrow-io-compressed-test
16/51 Test apache#16: arrow-io-compressed-test ............. Passed 3.48 sec
Start 17: arrow-io-file-test
17/51 Test apache#17: arrow-io-file-test ................... Passed 0.74 sec
Start 18: arrow-io-hdfs-test
18/51 Test apache#18: arrow-io-hdfs-test ................... Passed 0.12 sec
Start 19: arrow-io-memory-test
19/51 Test apache#19: arrow-io-memory-test ................. Passed 2.77 sec
Start 20: arrow-utility-test
20/51 Test apache#20: arrow-utility-test ...................***Failed 5.65 sec
Start 21: arrow-threading-utility-test
21/51 Test apache#21: arrow-threading-utility-test ......... Passed 1.34 sec
Start 22: arrow-compute-compute-test
22/51 Test apache#22: arrow-compute-compute-test ........... Passed 0.13 sec
Start 23: arrow-compute-boolean-test
23/51 Test apache#23: arrow-compute-boolean-test ........... Passed 0.15 sec
Start 24: arrow-compute-cast-test
24/51 Test apache#24: arrow-compute-cast-test .............. Passed 0.22 sec
Start 25: arrow-compute-hash-test
25/51 Test apache#25: arrow-compute-hash-test .............. Passed 2.61 sec
Start 26: arrow-compute-isin-test
26/51 Test apache#26: arrow-compute-isin-test .............. Passed 0.81 sec
Start 27: arrow-compute-match-test
27/51 Test apache#27: arrow-compute-match-test ............. Passed 0.40 sec
Start 28: arrow-compute-sort-to-indices-test
28/51 Test apache#28: arrow-compute-sort-to-indices-test ... Passed 3.33 sec
Start 29: arrow-compute-nth-to-indices-test
29/51 Test apache#29: arrow-compute-nth-to-indices-test .... Passed 1.51 sec
Start 30: arrow-compute-util-internal-test
30/51 Test apache#30: arrow-compute-util-internal-test ..... Passed 0.13 sec
Start 31: arrow-compute-add-test
31/51 Test apache#31: arrow-compute-add-test ............... Passed 0.12 sec
Start 32: arrow-compute-aggregate-test
32/51 Test apache#32: arrow-compute-aggregate-test ......... Passed 14.70 sec
Start 33: arrow-compute-compare-test
33/51 Test apache#33: arrow-compute-compare-test ........... Passed 7.96 sec
Start 34: arrow-compute-take-test
34/51 Test apache#34: arrow-compute-take-test .............. Passed 4.80 sec
Start 35: arrow-compute-filter-test
35/51 Test apache#35: arrow-compute-filter-test ............ Passed 8.23 sec
Start 36: arrow-dataset-dataset-test
36/51 Test apache#36: arrow-dataset-dataset-test ........... Passed 0.25 sec
Start 37: arrow-dataset-discovery-test
37/51 Test apache#37: arrow-dataset-discovery-test ......... Passed 0.13 sec
Start 38: arrow-dataset-file-ipc-test
38/51 Test apache#38: arrow-dataset-file-ipc-test .......... Passed 0.21 sec
Start 39: arrow-dataset-file-test
39/51 Test apache#39: arrow-dataset-file-test .............. Passed 0.12 sec
Start 40: arrow-dataset-filter-test
40/51 Test apache#40: arrow-dataset-filter-test ............ Passed 0.16 sec
Start 41: arrow-dataset-partition-test
41/51 Test apache#41: arrow-dataset-partition-test ......... Passed 0.13 sec
Start 42: arrow-dataset-scanner-test
42/51 Test apache#42: arrow-dataset-scanner-test ........... Passed 0.20 sec
Start 43: arrow-filesystem-test
43/51 Test apache#43: arrow-filesystem-test ................ Passed 1.62 sec
Start 44: arrow-hdfs-test
44/51 Test apache#44: arrow-hdfs-test ...................... Passed 0.13 sec
Start 45: arrow-feather-test
45/51 Test apache#45: arrow-feather-test ................... Passed 0.91 sec
Start 46: arrow-ipc-read-write-test
46/51 Test apache#46: arrow-ipc-read-write-test ............ Passed 5.77 sec
Start 47: arrow-ipc-json-simple-test
47/51 Test apache#47: arrow-ipc-json-simple-test ........... Passed 0.16 sec
Start 48: arrow-ipc-json-test
48/51 Test apache#48: arrow-ipc-json-test .................. Passed 0.27 sec
Start 49: arrow-json-integration-test
49/51 Test apache#49: arrow-json-integration-test .......... Passed 0.13 sec
Start 50: arrow-json-test
50/51 Test apache#50: arrow-json-test ...................... Passed 0.26 sec
Start 51: arrow-orc-adapter-test
51/51 Test apache#51: arrow-orc-adapter-test ............... Passed 1.92 sec
98% tests passed, 1 tests failed out of 51
Label Time Summary:
arrow-tests = 27.38 sec (27 tests)
arrow_compute = 45.11 sec (14 tests)
arrow_dataset = 1.21 sec (7 tests)
arrow_ipc = 6.20 sec (3 tests)
unittest = 79.91 sec (51 tests)
Total Test time (real) = 79.99 sec
The following tests FAILED:
20 - arrow-utility-test (Failed)
Errors while running CTest
```
Closesapache#7142 from kiszk/ARROW-8754
Authored-by: Kazuaki Ishizaki <ishizaki@jp.ibm.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
pitrou added a commit that referenced this pull request Apr 7, 2021
From a deadlocked run...
```
#0 0x00007f8a5d48dccd in __lll_lock_wait () from /lib64/libpthread.so.0
#1 0x00007f8a5d486f05 in pthread_mutex_lock () from /lib64/libpthread.so.0
#2 0x00007f8a566e7e89 in arrow::internal::FnOnce<void ()>::FnImpl<arrow::Future<Aws::Utils::Outcome<Aws::S3::Model::ListObjectsV2Result, Aws::S3::S3Error> >::Callback<arrow::fs::(anonymous namespace)::TreeWalker::ListObjectsV2Handler> >::invoke() () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#3 0x00007f8a5650efa0 in arrow::FutureImpl::AddCallback(arrow::internal::FnOnce<void ()>) () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#4 0x00007f8a566e67a9 in arrow::fs::(anonymous namespace)::TreeWalker::ListObjectsV2Handler::SpawnListObjectsV2() () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#5 0x00007f8a566e723f in arrow::fs::(anonymous namespace)::TreeWalker::WalkChild(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, int) () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#6 0x00007f8a566e827d in arrow::internal::FnOnce<void ()>::FnImpl<arrow::Future<Aws::Utils::Outcome<Aws::S3::Model::ListObjectsV2Result, Aws::S3::S3Error> >::Callback<arrow::fs::(anonymous namespace)::TreeWalker::ListObjectsV2Handler> >::invoke() () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#7 0x00007f8a5650efa0 in arrow::FutureImpl::AddCallback(arrow::internal::FnOnce<void ()>) () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#8 0x00007f8a566e67a9 in arrow::fs::(anonymous namespace)::TreeWalker::ListObjectsV2Handler::SpawnListObjectsV2() () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#9 0x00007f8a566e723f in arrow::fs::(anonymous namespace)::TreeWalker::WalkChild(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, int) () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
#10 0x00007f8a566e74b1 in arrow::fs::(anonymous namespace)::TreeWalker::DoWalk() () from /arrow/r/check/arrow.Rcheck/arrow/libs/arrow.so
```
The callback `ListObjectsV2Handler` is being called recursively and the mutex is non-reentrant thus deadlock.
To fix it I got rid of the mutex on `TreeWalker` by using `arrow::util::internal::TaskGroup` instead of manually tracking the #/status of in-flight requests.
Closesapache#9842 from westonpace/bugfix/arrow-12040
Lead-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
pitrou pushed a commit that referenced this pull request Jun 16, 2021
Before change:
```
Direct leak of 65536 byte(s) in 1 object(s) allocated from:
#0 0x522f09 in
#1 0x7f28ae5826f4 in
#2 0x7f28ae57fa5d in
#3 0x7f28ae58cb0f in
#4 0x7f28ae58bda0 in
...
```
After change:
```
Direct leak of 65536 byte(s) in 1 object(s) allocated from:
#0 0x522f09 in posix_memalign (/build/cpp/debug/arrow-dataset-file-csv-test+0x522f09)
#1 0x7f28ae5826f4 in arrow::(anonymous namespace)::SystemAllocator::AllocateAligned(long, unsigned char**) /arrow/cpp/src/arrow/memory_pool.cc:213:24
#2 0x7f28ae57fa5d in arrow::BaseMemoryPoolImpl<arrow::(anonymous namespace)::SystemAllocator>::Allocate(long, unsigned char**) /arrow/cpp/src/arrow/memory_pool.cc:405:5
#3 0x7f28ae58cb0f in arrow::PoolBuffer::Reserve(long) /arrow/cpp/src/arrow/memory_pool.cc:717:9
#4 0x7f28ae58bda0 in arrow::PoolBuffer::Resize(long, bool) /arrow/cpp/src/arrow/memory_pool.cc:741:7
...
```
Closesapache#10498 from westonpace/feature/ARROW-13027--c-fix-asan-stack-traces-in-ci
Authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for freeto 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.

2 participants

@nealrichardson@pitrou