GH-35344: [C++][Format] Implementation of the LIST_VIEW and LARGE_LIST_VIEW array formats #35345

felipecrv · 2023-04-26T14:17:46Z

Rationale for this change

Mailing list discussion: https://lists.apache.org/thread/r28rw5n39jwtvn08oljl09d4q2c1ysvb

What changes are included in this PR?

Initial implementation of the new format in C++.

Are these changes tested?

Unit tests being written on every commit adding new functionality. More needs to be implemented for Integration Tests (required) to be implementable.

Are there any user-facing changes?

A new array format. It should have no impact for users that don't use it.

Closes: [C++][Format] Draft an implementation of the LIST_VIEW array format #35344

github-actions · 2023-04-26T14:18:10Z

Closes: [C++][Format] Draft an implementation of the LIST_VIEW array format #35344

github-actions · 2023-04-26T14:18:13Z

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

felipecrv · 2023-04-26T15:40:09Z

@bkietz

… by the final list-view spec

…equired by the final list-view spec

felipecrv · 2023-11-17T02:54:55Z

cpp/src/arrow/array/concatenate_test.cc

+    return rag_.ArrayOf(std::move(type), size, null_probability);
+  }
+
+  // TODO(GH-38656): Use the random array generators from testing/random.h here


@pitrou I isolated all the random-generation code in this class and removed the complicated List[View]ConcatenationChecker templates.

cpp/src/arrow/array/concatenate_test.cc

felipecrv · 2023-11-21T15:50:49Z

@pitrou

pitrou

Sorry for finding two more nits. Feel free to ping when done!

pitrou · 2023-11-21T16:44:07Z

cpp/src/arrow/array/concatenate.cc

+        if (sizes[position] > 0) {
+          // NOTE: Concatenate can be called during IPC reads to append delta
+          // dictionaries. Avoid UB on non-validated input by doing the addition in the
+          // unsigned domain. (the result can later be validated using
+          // Array::ValidateFull)
+          const auto displaced_offset = SafeSignedAdd(offsets[position], displacement);
+          // displaced_offset>=0 is guaranteed by RangeOfValuesUsed returning the
+          // smallest offset of valid and non-empty list-views.
+          DCHECK_GE(displaced_offset, 0);
+          dst[position] = displaced_offset;
+        } else {
+          // Do nothing to leave dst[position] as 0.
+        }


I might be misreading, but is it just the same as visit_not_null(i)?

You're right. I extracted the function from below when I noticed the dup, but forgot to do the reverse-inlining above.

Pushing soon.

cpp/src/arrow/array/concatenate_test.cc

cpp/src/arrow/util/list_util.h

…equired by the final list-view spec

pitrou · 2023-11-22T15:04:59Z

@felipecrv We'll want to update https://github.com/apache/arrow/blob/main/docs/source/status.rst in a followup PR.

felipecrv · 2023-11-22T15:19:57Z

@felipecrv We'll want to update https://github.com/apache/arrow/blob/main/docs/source/status.rst in a followup PR.

I will be extremely glad to send that PR.

mapleFU · 2023-11-22T16:16:29Z

bravo 🍺!

mapleFU · 2023-11-22T16:23:06Z

I've create an issue about parquet. #38849

conbench-apache-arrow · 2023-11-22T19:33:57Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 8cc71ab.

There were 5 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-11-22 16:59:04Z
- file-write (Python) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=parquet, input_type=dataframe
- dataset-selectivity (Python) with dataset=nyctaxi_multi_ipc_s3, selectivity=10%
and 3 more (see the report linked below)

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

…pache#37877) ### Rationale for this change More details in the draft implementations of this spec: - C++: apache#35345 - Go: apache#37468 ### What changes are included in this PR? - Some unrelated fixes to the spec text (I can extract these to another PR if necessary) - Changes to the spec text - Additions to the Flatbuffers specifications of the Arrow format ### Are these changes tested? N/A. ### Are there any user-facing changes? Changes in documentation and backwards compatible additions to the format spec. * Closes: apache#37876 Lead-authored-by: Felipe Oliveira Carvalho <[email protected]> Co-authored-by: David Li <[email protected]> Co-authored-by: Antoine Pitrou <[email protected]> Co-authored-by: Benjamin Kietzman <[email protected]> Signed-off-by: Matt Topol <[email protected]>

…E_LIST_VIEW array formats (apache#37468) ### Rationale for this change Go implementation of apache#35345. ### What changes are included in this PR? - [x] Add `LIST_VIEW` and `LARGE_LIST_VIEW` to datatype.go - [x] Add `ListView` and `LargeListView` to list.go - [x] Add `ListViewType` and `LargeListViewType` to datatype_nested.go - [x] Add list-view builders - [x] Implement list-view comparison in compare.go - [x] String conversion in both directions - [x] Validation of list-view arrays - [x] Generation of random list-view arrays - [x] Concatenation of list-view arrays in concat.go - [x] JSON serialization/deserialization - [x] Add data used for tests in `arrdata.go` - [x] Add Flatbuffer changes - [x] Add IPC support ### Are these changes tested? Yes. Existing tests are being changed to also cover list-view variations as well as new tests focused solely on the list-view format. ### Are there any user-facing changes? New structs and functions introduced. * Closes: apache#35344 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Matt Topol <[email protected]>

…GE_LIST_VIEW array formats (apache#35345) ### Rationale for this change Mailing list discussion: https://lists.apache.org/thread/r28rw5n39jwtvn08oljl09d4q2c1ysvb ### What changes are included in this PR? Initial implementation of the new format in C++. ### Are these changes tested? Unit tests being written on every commit adding new functionality. More needs to be implemented for Integration Tests (required) to be implementable. ### Are there any user-facing changes? A new array format. It should have no impact for users that don't use it. * Closes: apache#35344 Authored-by: Felipe Oliveira Carvalho <[email protected]> Signed-off-by: Antoine Pitrou <[email protected]>

github-actions bot added Component: C++ Component: Parquet Component: Python awaiting review Awaiting review labels Apr 26, 2023

felipecrv force-pushed the list_view branch from 0be29e3 to 8feebe4 Compare April 26, 2023 15:06

felipecrv force-pushed the list_view branch 2 times, most recently from 3204c80 to 5b0944c Compare April 28, 2023 23:59

felipecrv force-pushed the list_view branch 3 times, most recently from 90ce26e to f3a325a Compare May 12, 2023 14:59

felipecrv force-pushed the list_view branch from f3a325a to 09d9d2f Compare June 13, 2023 02:08

felipecrv force-pushed the list_view branch from 09d9d2f to 159b6f0 Compare July 5, 2023 02:44

felipecrv force-pushed the list_view branch 4 times, most recently from 06ca3f2 to 2c21e52 Compare July 20, 2023 03:34

felipecrv force-pushed the list_view branch 11 times, most recently from b4c6992 to 5e3a24b Compare August 8, 2023 23:03

felipecrv added 4 commits November 16, 2023 23:21

concatenate_test.cc: Isolate all the random array generation code

d42aae8

concatenate.cc: Preserve the properties of offsets and sizes required…

252ff8b

… by the final list-view spec

fixup! concatenate.cc: Preserve the properties of offsets and sizes r…

b30d2e5

…equired by the final list-view spec

fixup! fixup! [Large]ListViewScalar: Implement all operations

c2068bb

felipecrv commented Nov 17, 2023

View reviewed changes

cpp/src/arrow/array/concatenate_test.cc Show resolved Hide resolved

felipecrv added 4 commits November 17, 2023 00:03

fixup! concatenate_test.cc: Isolate all the random array generation code

a41490e

fixup! Make the C Bridge test set for list-views complete

79c6acd

fixup! list_util.h: Add RangeOfValuesUsed() function

05f6f38

fixup! concatenate_test.cc: Isolate all the random array generation code

773c800

felipecrv force-pushed the list_view branch from 0b93ffa to 773c800 Compare November 17, 2023 14:14

pitrou approved these changes Nov 21, 2023

View reviewed changes

wgtmac removed their request for review November 22, 2023 01:40

felipecrv added 2 commits November 22, 2023 11:52

fixup! list_util.h: Add SumOfLogicalListSizes() utility

ab0dcaf

fixup! concatenate.cc: Preserve the properties of offsets and sizes r…

62ede21

…equired by the final list-view spec

pitrou merged commit 8cc71ab into apache:main Nov 22, 2023
34 of 35 checks passed

pitrou removed the awaiting change review Awaiting change review label Nov 22, 2023

This was referenced Dec 21, 2023

[CI][Python][Packaging] Failed to build wheel for Windows since ListView merge #39333

Closed

GH-39333: [C++] Don't use "if constexpr" in lambda #39334

Merged

danepitkin mentioned this pull request Jan 26, 2024

[Python] Add scaffolding for ListView and LargeListView array formats #39812

Closed

thisisnic mentioned this pull request Jan 30, 2024

[C++] Parquet reader is unable to read LargeString columns #39682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-35344: [C++][Format] Implementation of the LIST_VIEW and LARGE_LIST_VIEW array formats #35345

GH-35344: [C++][Format] Implementation of the LIST_VIEW and LARGE_LIST_VIEW array formats #35345

felipecrv commented Apr 26, 2023 •

edited by github-actions bot

Loading

github-actions bot commented Apr 26, 2023

github-actions bot commented Apr 26, 2023

felipecrv commented Apr 26, 2023

felipecrv Nov 17, 2023

felipecrv commented Nov 21, 2023

pitrou left a comment

pitrou Nov 21, 2023

felipecrv Nov 22, 2023

pitrou commented Nov 22, 2023

felipecrv commented Nov 22, 2023

mapleFU commented Nov 22, 2023

mapleFU commented Nov 22, 2023

conbench-apache-arrow bot commented Nov 22, 2023

GH-35344: [C++][Format] Implementation of the LIST_VIEW and LARGE_LIST_VIEW array formats #35345

GH-35344: [C++][Format] Implementation of the LIST_VIEW and LARGE_LIST_VIEW array formats #35345

Conversation

felipecrv commented Apr 26, 2023 • edited by github-actions bot Loading

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 Apr 26, 2023

github-actions bot commented Apr 26, 2023

felipecrv commented Apr 26, 2023

felipecrv Nov 17, 2023

Choose a reason for hiding this comment

felipecrv commented Nov 21, 2023

pitrou left a comment

Choose a reason for hiding this comment

pitrou Nov 21, 2023

Choose a reason for hiding this comment

felipecrv Nov 22, 2023

Choose a reason for hiding this comment

pitrou commented Nov 22, 2023

felipecrv commented Nov 22, 2023

mapleFU commented Nov 22, 2023

mapleFU commented Nov 22, 2023

conbench-apache-arrow bot commented Nov 22, 2023

felipecrv commented Apr 26, 2023 •

edited by github-actions bot

Loading