GH-40270: [C++] Use LargeStringArray for casting when writing tables to CSV #40271

sighingnow · 2024-02-28T03:12:56Z

Rationale for this change

Avoid casting failures when tables contains too long large string arrays.

What changes are included in this PR?

Replace the usage of StringArray to LargeStringArray.

Are these changes tested?

No extra test case is needed (as it is to fix some corner cases).

Are there any user-facing changes?

No user-facing changes.

GitHub Issue: [C++] CSV writer's WriteTable failed with: Failed casting from large_string to string: input array too large #40270

github-actions · 2024-02-28T03:13:22Z

⚠️ GitHub issue #40270 has been automatically assigned in GitHub to PR creator.

pitrou · 2024-02-28T11:41:07Z

CI results indicate segfaults, surely something is wrong in this PR?

sighingnow · 2024-02-28T13:41:57Z

CI results indicate segfaults, surely something is wrong in this PR?

Issue fixed, and has passed the arrow-csv-test in local environment.

sighingnow · 2024-02-28T15:06:58Z

The timeout error of s3 test shouldn't be caused by this PR: https://github.com/apache/arrow/actions/runs/8081349229/job/22079702366

pitrou · 2024-02-28T15:30:17Z

After running the benchmarks locally, it seems that this is triggering a bunch of regressions when writing a StringArray to CSV:

-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
Regressions: (8)
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
                   benchmark        baseline       contender  change %                                                                                                                                                                                         counters
    WriteCsvStringNoQuote/50 692.326 MiB/sec 621.875 MiB/sec   -10.176     {'family_index': 1, 'per_family_instance_index': 3, 'run_name': 'WriteCsvStringNoQuote/50', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 2217, 'null_percent': 50.0}
WriteCsvStringRejectQuote/50 704.688 MiB/sec 625.617 MiB/sec   -11.221 {'family_index': 3, 'per_family_instance_index': 3, 'run_name': 'WriteCsvStringRejectQuote/50', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 2418, 'null_percent': 50.0}
    WriteCsvStringNoQuote/10   1.478 GiB/sec   1.250 GiB/sec   -15.420     {'family_index': 1, 'per_family_instance_index': 2, 'run_name': 'WriteCsvStringNoQuote/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 2840, 'null_percent': 10.0}
     WriteCsvStringNoQuote/0   1.976 GiB/sec   1.652 GiB/sec   -16.403       {'family_index': 1, 'per_family_instance_index': 0, 'run_name': 'WriteCsvStringNoQuote/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 3384, 'null_percent': 0.0}
     WriteCsvStringNoQuote/1   1.857 GiB/sec   1.540 GiB/sec   -17.098       {'family_index': 1, 'per_family_instance_index': 1, 'run_name': 'WriteCsvStringNoQuote/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 3184, 'null_percent': 1.0}
WriteCsvStringRejectQuote/10   1.829 GiB/sec   1.465 GiB/sec   -19.942 {'family_index': 3, 'per_family_instance_index': 2, 'run_name': 'WriteCsvStringRejectQuote/10', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 3843, 'null_percent': 10.0}
 WriteCsvStringRejectQuote/0   2.878 GiB/sec   2.300 GiB/sec   -20.086   {'family_index': 3, 'per_family_instance_index': 0, 'run_name': 'WriteCsvStringRejectQuote/0', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 5652, 'null_percent': 0.0}
 WriteCsvStringRejectQuote/1   2.660 GiB/sec   1.966 GiB/sec   -26.074   {'family_index': 3, 'per_family_instance_index': 1, 'run_name': 'WriteCsvStringRejectQuote/1', 'repetitions': 1, 'repetition_index': 0, 'threads': 1, 'iterations': 4869, 'null_percent': 1.0}

sighingnow · 2024-02-28T16:00:34Z

10% looks a bit significant, how could I replay the regression benchmark? Could you please help show me the command line?

pitrou · 2024-02-28T16:01:57Z

You can build in release mode and pass -DARROW_BUILD_BENCHMARKS=ON to CMake. You'll then find some benchmark executables in the target directory, including arrow-csv-writer-benchmark.

pitrou · 2024-02-28T16:06:48Z

I think this means that this should be more careful when casting. Only cast to large string if the input data is too large for a regular string array.

sighingnow · 2024-02-28T17:05:31Z

I think this means that this should be more careful when casting. Only cast to large string if the input data is too large for a regular string array.

Thanks for the suggestion, indeed that is exactly what I was considering. I have pushed an implementation which tries to cast to StringArray then LargeStringArray if failed and dispatches to a templated implementations based on the array's type in runtime. As virtual member method cannot be template, this is the simplest / least changed solution I can think of.

I failed to find how to run regression benchmarks, but I have run the main branch and this PR's branch twice, the numbers looks good now.

sighingnow · 2024-03-01T03:07:08Z

Hi @pitrou, I would like to if there are further comments on this pull request?

Thanks!

bkietz · 2024-03-01T15:06:53Z

@ursabot please benchmark

ursabot · 2024-03-01T15:06:59Z

Benchmark runs are scheduled for commit 7e7e5c6. Watch https://buildkite.com/apache-arrow and https://conbench.ursa.dev for updates. A comment will be posted here when the runs are complete.

pitrou · 2024-03-01T16:43:36Z

cpp/src/arrow/csv/writer.cc

-        std::shared_ptr<Array> casted,
-        compute::Cast(data, /*to_type=*/utf8(), compute::CastOptions(), &ctx));
-    casted_array_ = checked_pointer_cast<StringArray>(casted);
+    auto casted = compute::Cast(data, /*to_type=*/utf8(), compute::CastOptions(), &ctx);


Perhaps not very important, but I think the algorithm should ideally be:

if the input data is large_binary or large_utf8, cast it to large_utf8

otherwise, try to cast it to utf8, and fallback to large_utf8

conbench-apache-arrow · 2024-03-01T19:46:04Z

Thanks for your patience. Conbench analyzed the 7 benchmarking runs that have been run so far on PR commit 7e7e5c6.

There was 1 benchmark result indicating a performance regression:

Pull Request Run on ursa-thinkcentre-m75q at 2024-03-01 18:23:09Z
- UniqueString100bytes (C++) with params=0, source=cpp-micro, suite=arrow-compute-vector-hash-benchmark

The full Conbench report has more details.

sighingnow · 2024-03-04T06:46:35Z

UniqueString100bytes (C++) with params=0, source=cpp-micro, suite=arrow-compute-vector-hash-benchmark

The regression of arrow-compute-vector-hash-benchmark should be caused by this PR.

sighingnow · 2024-03-04T07:29:46Z

The CI failure shouldn't be caused by this PR:

================================== FAILURES ===================================
_______________________ test_dateutil_tzinfo_to_string ________________________
    def test_dateutil_tzinfo_to_string():
        pytest.importorskip("dateutil")
        import dateutil.tz
    
        tz = dateutil.tz.UTC
        assert pa.lib.tzinfo_to_string(tz) == 'UTC'
        tz = dateutil.tz.gettz('Europe/Paris')
>       assert pa.lib.tzinfo_to_string(tz) == 'Europe/Paris'
E       AssertionError: assert 'Europe/Monaco' == 'Europe/Paris'
E         - Europe/Paris
E         + Europe/Monaco
pyarrow\tests\test_types.py:355: AssertionError
============================== warnings summary ===============================

bkietz · 2024-03-04T13:55:58Z

cpp/src/arrow/csv/writer.cc

@@ -108,6 +108,17 @@ int64_t CountQuotes(std::string_view s) {
 constexpr int64_t kQuoteCount = 2;
 constexpr int64_t kQuoteDelimiterCount = kQuoteCount + /*end_char*/ 1;

+#ifndef CSV_WRITER_DISPATCH_STRING_ARRAY_TYPE
+#define CSV_WRITER_DISPATCH_STRING_ARRAY_TYPE(array, func, ...) \


Please replace this macro with usage of VisitArrayInline or so

I'm afraid it would be a bit over-design. There's actually two method PopulateRows and UpdateRowLengths that would require such a dispatch. That means, I would need to define four visitor classes for two different methods in two different classes.

Any ideas? @bkietz

Kindly ping @bkietz, do you think if we would just keep current implementation, for simplicity?

Thanks!

Well, a visitor might be overkill, but it would be better to remove the macro and write the code explicitly, IMHO.

I also thinks a template helper function or just write the code is a better way

Fixed. @pitrou could please help to take another look? thanks!

jingshi-ant · 2024-03-29T08:43:58Z

hello，will this issue be resolved in the near future? we encountered the same problem.

mapleFU · 2024-03-30T03:57:19Z

Would you mind rebase this pr? It might fix some ci problem(and introduce some new)

sighingnow · 2024-03-30T05:07:58Z

Would you mind rebase this pr? It might fix some ci problem(and introduce some new)

Rebased against current main.

mapleFU · 2024-04-09T14:14:22Z

cpp/src/arrow/csv/writer.cc

@@ -108,6 +108,17 @@ int64_t CountQuotes(std::string_view s) {
 constexpr int64_t kQuoteCount = 2;
 constexpr int64_t kQuoteDelimiterCount = kQuoteCount + /*end_char*/ 1;

+#ifndef CSV_WRITER_DISPATCH_STRING_ARRAY_TYPE
+#define CSV_WRITER_DISPATCH_STRING_ARRAY_TYPE(array, func, ...) \


I also thinks a template helper function or just write the code is a better way

mapleFU · 2024-04-09T14:16:27Z

cpp/src/arrow/csv/writer.cc

@@ -146,7 +165,7 @@ class ColumnPopulator {

 protected:
  virtual Status UpdateRowLengths(int64_t* row_lengths) = 0;
-  std::shared_ptr<StringArray> casted_array_;
+  std::shared_ptr<Array> array_;


Can you just a comment for array_ that it would be a StringArray or LargeStringArray?

mapleFU · 2024-04-09T14:17:14Z

cpp/src/arrow/csv/writer.cc

+    } else {
+      auto casted = compute::Cast(data, /*to_type=*/utf8(), compute::CastOptions(), &ctx);
+      if (casted.ok()) {
+        array_ = casted.ValueOrDie();


Suggested change

array_ = casted.ValueOrDie();

array_ = std::move(casted).ValueOrDie();

cpp/src/arrow/csv/writer.cc

mapleFU · 2024-04-09T14:21:26Z

cpp/src/arrow/csv/writer.cc

+    } else {                                                    \
+      return func<LargeStringArray>(__VA_ARGS__);               \


Also dcheck the type is large_string here?

mapleFU

+1 for me, besides, would it be hard for a testing?

At least we can use a large string array (with short strings in it) as input?

sighingnow · 2024-04-11T06:28:09Z

+1 for me, besides, would it be hard for a testing?

At least we can use a large string array (with short strings in it) as input?

Testcase added.

sighingnow · 2024-04-11T10:55:36Z

Hi @mapleFU @pitrou , could you please take another look and help merge this PR?

Thanks!

mapleFU · 2024-04-11T10:57:29Z

This looks ok to me, but I'll wait for @pitrou @bkietz 's idea

pitrou

LGTM except for one minor comment

cpp/src/arrow/csv/writer.cc

Signed-off-by: Tao He <sighingnow@gmail.com>

Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Tao He <sighingnow@gmail.com>

pitrou · 2024-04-11T15:22:20Z

@github-actions crossbow submit -g cpp

github-actions · 2024-04-11T15:24:52Z

Revision: ff4f7ea

Submitted crossbow builds: ursacomputing/crossbow @ actions-8c63406bed

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

pitrou · 2024-04-11T16:13:15Z

@github-actions crossbow submit -g cpp

github-actions · 2024-04-11T16:15:45Z

Revision: ff4f7ea

Submitted crossbow builds: ursacomputing/crossbow @ actions-547e0b6dcb

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-cuda-cpp
test-debian-12-cpp-amd64
test-debian-12-cpp-i386
test-fedora-39-cpp
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-emscripten
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

conbench-apache-arrow · 2024-04-11T23:13:54Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 7288bd5.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 3 possible false positives for unstable benchmarks that are known to sometimes produce them.

…ables to CSV (apache#40271) ### Rationale for this change Avoid casting failures when tables contains too long large string arrays. ### What changes are included in this PR? Replace the usage of `StringArray` to `LargeStringArray`. ### Are these changes tested? No extra test case is needed (as it is to fix some corner cases). ### Are there any user-facing changes? No user-facing changes. * GitHub Issue: apache#40270 Lead-authored-by: Tao He <sighingnow@gmail.com> Co-authored-by: Antoine Pitrou <antoine@python.org> Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Antoine Pitrou <antoine@python.org>

github-actions bot added Component: C++ awaiting review Awaiting review labels Feb 28, 2024

sighingnow force-pushed the ht/fixes-csv-string-array branch from 91069ee to da7c8cb Compare February 28, 2024 03:14

sighingnow mentioned this pull request Feb 28, 2024

bug(analytical): context.output raise error when result is huge alibaba/GraphScope#3578

Closed

sighingnow force-pushed the ht/fixes-csv-string-array branch from da7c8cb to 75ba58b Compare February 28, 2024 13:41

sighingnow force-pushed the ht/fixes-csv-string-array branch from 75ba58b to 7e7e5c6 Compare February 28, 2024 17:02

pitrou reviewed Mar 1, 2024

View reviewed changes

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 1, 2024

sighingnow force-pushed the ht/fixes-csv-string-array branch from 487cabb to a0dc29b Compare March 4, 2024 06:49

bkietz requested changes Mar 4, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting committer review Awaiting committer review labels Mar 4, 2024

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Mar 30, 2024

mapleFU reviewed Apr 9, 2024

View reviewed changes

sighingnow force-pushed the ht/fixes-csv-string-array branch 2 times, most recently from 7e1d00f to dcf7100 Compare April 11, 2024 04:18

mapleFU approved these changes Apr 11, 2024

View reviewed changes

sighingnow force-pushed the ht/fixes-csv-string-array branch from dcf7100 to b492768 Compare April 11, 2024 06:27

pitrou requested changes Apr 11, 2024

View reviewed changes

cpp/src/arrow/csv/writer.cc Outdated Show resolved Hide resolved

sighingnow force-pushed the ht/fixes-csv-string-array branch from ea8d2b1 to 05ef5c1 Compare April 11, 2024 15:11

sighingnow and others added 3 commits April 11, 2024 17:20

Use LargeStringArray for casting when writing tables to CSV

a08ced6

Signed-off-by: Tao He <sighingnow@gmail.com>

Avoidd casting the array pointer.

1934a16

Co-authored-by: Antoine Pitrou <pitrou@free.fr> Signed-off-by: Tao He <sighingnow@gmail.com>

Integrate large_utf8 test into regular parameterized tests

ff4f7ea

pitrou force-pushed the ht/fixes-csv-string-array branch from 457aa1c to ff4f7ea Compare April 11, 2024 15:20

pitrou approved these changes Apr 11, 2024

View reviewed changes

pitrou merged commit 7288bd5 into apache:main Apr 11, 2024
36 checks passed

pitrou removed the awaiting change review Awaiting change review label Apr 11, 2024

pitrou mentioned this pull request Apr 11, 2024

[C++] CSV writer's WriteTable failed with: Failed casting from large_string to string: input array too large #40270

Closed

sighingnow deleted the ht/fixes-csv-string-array branch April 12, 2024 01:17

	array_ = casted.ValueOrDie();
	array_ = std::move(casted).ValueOrDie();

GH-40270: [C++] Use LargeStringArray for casting when writing tables to CSV #40271

GH-40270: [C++] Use LargeStringArray for casting when writing tables to CSV #40271

Conversation

sighingnow commented Feb 28, 2024 • edited by github-actions bot

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Feb 28, 2024

pitrou commented Feb 28, 2024

sighingnow commented Feb 28, 2024

sighingnow commented Feb 28, 2024

pitrou commented Feb 28, 2024

sighingnow commented Feb 28, 2024 • edited

pitrou commented Feb 28, 2024

pitrou commented Feb 28, 2024

sighingnow commented Feb 28, 2024

sighingnow commented Mar 1, 2024

bkietz commented Mar 1, 2024

ursabot commented Mar 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conbench-apache-arrow bot commented Mar 1, 2024

sighingnow commented Mar 4, 2024

sighingnow commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingshi-ant commented Mar 29, 2024

mapleFU commented Mar 30, 2024

sighingnow commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mapleFU left a comment • edited

Choose a reason for hiding this comment

sighingnow commented Apr 11, 2024

sighingnow commented Apr 11, 2024

mapleFU commented Apr 11, 2024

pitrou left a comment

Choose a reason for hiding this comment

pitrou commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

pitrou commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

conbench-apache-arrow bot commented Apr 11, 2024

sighingnow commented Feb 28, 2024 •

edited by github-actions bot

sighingnow commented Feb 28, 2024 •

edited

mapleFU left a comment •

edited