ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class #13521

wesm · 2022-07-05T21:10:56Z

This was a pretty painful refactoring project, but all for the better long term.

I will do my best to improve the PR summary to explain changes as I go through the PR myself, but the basic summary is that argument shape is no longer part of kernel signatures (or expressions or anything else) -- the "all scalar" evaluation case is handled by promoting scalars to ArraySpan of length 1. This enabled the deletion of many "duplicate" kernel implementations and uncovered some bugs in some kernel implementations as a result of passing many sliced length-1 inputs up-promoted from scalars.

This had painful knock-on effects because some scalar values -- sparse unions and some other non-primitive types -- were not "well-formed" enough to be able to be converted to ArraySpan without needing to allocate memory. As a result, I had to change the requirements (and in the case of sparse unions, the representation to correspond to a "single row view" of a SparseUnionArray -- i.e. the "value" for a sparse union is now a vector of scalars rather than a single scalar, even though only one of the values in the vector is the "active" scalar) of some scalar values to not allow null values. We should / could consider implementing scalars altogether internally as arrays of length 1 but we can discuss that as a follow up.

Some other interesting stuff in here is the new experimental arrow::TypeHolder struct which allows us to pass around either const DataType* or std::shared_ptr<DataType> in contexts where type ownership isn't strictly necessary. So now we can deal only with raw pointers when doing type signature checking / kernel dispatching, for example. We should look at refactoring other code where we deal with shared_ptr<DataType> (but don't necessarily need to preserve ownership) to use TypeHolder.

There's some unfinished business here:

The GLib bindings must be refactored to follow this work

github-actions · 2022-07-05T21:11:17Z

https://issues.apache.org/jira/browse/ARROW-16757

cpp/src/arrow/array/builder_nested.h

cpp/src/arrow/array/data.h

cpp/src/arrow/compute/exec.cc

wesm · 2022-07-05T21:15:29Z

cpp/src/arrow/compute/exec.h

    for (const auto& value : this->values) {
-      result.emplace_back(value.descr());
+      result.emplace_back(value.type());


There are several places where we can probably drop to construct from all const DataType* which is less expensive, this is one of them (I need to check that it doesn't cause tests to fail)

wesm · 2022-07-05T21:15:54Z

cpp/src/arrow/compute/exec.h

  ArraySpan array;
-  const Scalar* scalar;
+  const Scalar* scalar = NULLPTR;


I applied Antoine's suggested optimization

cpp/src/arrow/compute/kernel.h

cpp/src/arrow/compute/kernels/vector_cumulative_ops_test.cc

cpp/src/arrow/scalar.h

cpp/src/arrow/type_fwd.h

wesm · 2022-07-05T21:54:30Z

@kou if my proposed solution to the SparseUnionScalar issue does not cause too much controversy (maybe wait a day or two for @pitrou or @lidavidm to look), I would appreciate your assistance refactoring the glib bindings to follow

wesm · 2022-07-06T00:35:13Z

I was able to fix the R unit tests (it was just error messages that don't have shapes in them anymore), so I'll work on getting the other CI jobs to fail and then I think the glib stuff is that only thing that will need fixing

kou · 2022-07-06T03:07:13Z

@wesm Sure! I'll do it.

cpp/src/arrow/type.h

cpp/src/arrow/scalar.h

cpp/src/arrow/scalar_test.cc

lidavidm · 2022-07-06T18:03:17Z

cpp/src/arrow/compute/function_internal.h

+    const std::shared_ptr<Scalar>& value) {
+  return value->type;
+}
+


It is rather complicated but it saves us a good amount of boilerplate to implement serialization/ToString/equality for all the options classes. Not sure if there's a better way (more codegen?)

wesm · 2022-07-06T19:28:12Z

This works. I suppose if we really wanted, we could keep a static buffer of zeroes and construct the span from slices of that based on type but that'd be quite a bit of work for one particular case.

Re: SparseUnionScalar — unfortunately we can't use a static buffer of zeros because child array types can have arbitrarily large values (e.g. fixed_size_binary can be arbitrarily large, or fixed_size_list, etc.)

wesm · 2022-07-06T19:30:01Z

58/70 Test #59: arrow-python-test .........................***Failed 0.22 sec
RuntimeError: module compiled against API version 0x10 but this version of numpy is 0xf

This is an odd failure in the MinGW 32/64 C++ builds, anyone know what that's about?

lidavidm · 2022-07-06T19:35:52Z

58/70 Test #59: arrow-python-test .........................***Failed 0.22 sec
RuntimeError: module compiled against API version 0x10 but this version of numpy is 0xf

This is an odd failure in the MinGW 32/64 C++ builds, anyone know what that's about?

@amol- you mentioned you noticed this earlier - did you figure out what happened there?

kou · 2022-07-07T01:17:44Z

I've pushed changes for GLib.

58/70 Test #59: arrow-python-test .........................***Failed 0.22 sec
RuntimeError: module compiled against API version 0x10 but this version of numpy is 0xf

This is an odd failure in the MinGW 32/64 C++ builds, anyone know what that's about?

I didn't notice it but it's not related to this change.

I think that it's caused by version change of NumPy installed by MSYS2 package manager and ccache.
MSYS2's NumPy package was updated to 1.23.0-1 from 1.22.4-3 recently. And this was caused from the update. (I think that https://github.com/apache/arrow/runs/7214056952?check_suite_focus=true is the first fail.)
It seems that ccache still uses build results with NumPy 1.22.4-3.

It seems that we can clear GitHub Actions cache manually by GitHub Actions Cache API: https://docs.github.com/en/rest/actions/cache
I'll try clearing GitHub Actions cache for MinGW 32/64 C++ builds.

kou · 2022-07-07T01:40:53Z

I'll try clearing GitHub Actions cache for MinGW 32/64 C++ builds.

I couldn't do this...

$ curl --verbose -X DELETE -H "Authorization: token ..." -H "Accept: application/vnd.github.v3+json" https://api.github.com/repos/apache/arrow/actions/caches/cpp-ccache-mingw64-cfa1684ec4da6404fc0c738c9563a75ca1148a7ca2e6fdfc6995e2833bbd0979
...
{
  "message": "Not Found",
  "documentation_url": "https://docs.github.com/rest/actions/cache#delete-a-github-actions-cache-for-a-repository-using-a-cache-id"
}

I think that we need to add NumPy version to cache key.

kou · 2022-07-07T01:53:59Z

I think that we need to add NumPy version to cache key.

#13534

pitrou

Overall I really like this and the scalar struct changes are acceptable IMHO.
Here are a bunch of assorted comments.

cpp/src/arrow/array/data.cc

cpp/src/arrow/array/data.h

cpp/src/arrow/array/data.cc

cpp/src/arrow/compute/exec.cc

pitrou · 2022-07-07T16:18:45Z

cpp/gdb_arrow.py

-class UnionScalarPrinter(ScalarPrinter):
+class SparseUnionScalarPrinter(ScalarPrinter):
+    """
+    Pretty-printer for arrow::UnionScalar and subclasses.


Suggested change

Pretty-printer for arrow::UnionScalar and subclasses.

Pretty-printer for arrow::SparseUnionScalar.

pitrou · 2022-07-07T16:19:13Z

cpp/gdb_arrow.py

+            return (f"{self._format_type()} of type {self.type}, "
+                    f"type code {type_code}, null value")
+        eval_values = StdVector(self.val['value'])
+        child_id = self.val['child_id'].cast(gdb.lookup_type('int'))


I don't think the cast is needed as it's already a C int?

(note: calls to gdb evaluation of C stuff can be costly, so this is not just pedantic)

pitrou · 2022-07-07T16:20:14Z

cpp/gdb_arrow.py

+
+
+
+class DenseUnionScalarPrinter(ScalarPrinter):
    """
    Pretty-printer for arrow::UnionScalar and subclasses.


Suggested change

Pretty-printer for arrow::UnionScalar and subclasses.

Pretty-printer for arrow::DenseUnionScalar and subclasses.

More refactoring More refactoring More refactoring Checkpoint, still some ValueDescr to remove Clean up and delete some more scalar output code More refactoring More refactoring More refactoring, code cleaning more cleaning More cleaning checkpoint Get everything compiling again More refactoring exec.cc refactoring, get compiling again Handle scalar -> array span promotions Work to make all scalars more well formed Make union scalars more 'well formed' All things compiling again Fix some more stuff Fix some more tedious errors Fix some more things Fixed MapBuilder::AppendArraySlice bug checkpoint Fix more bugs C++ tests passing again, restore cumulative_sum scalar tests

wesm · 2022-07-07T20:20:10Z

Thanks everyone. I'll merge this once the CI passes so I can get busy with the next items

kou · 2022-07-08T00:32:39Z

It seems that my change was lost by force-push. I'll add it again.

wesm · 2022-07-08T00:50:27Z

Oh sorry yes that’s my mistake

github-actions bot added Component: C++ Component: Python Component: R labels Jul 5, 2022

wesm commented Jul 5, 2022

View reviewed changes

wesm force-pushed the ARROW-16757-scalar-output-modality branch from 74d08eb to 908d88b Compare July 6, 2022 11:51

lidavidm approved these changes Jul 6, 2022

View reviewed changes

github-actions bot added the Component: GLib label Jul 7, 2022

pitrou reviewed Jul 7, 2022

View reviewed changes

wesm added 8 commits July 7, 2022 14:03

Fix Python bindings

85aa291

Fix R bindings compilation, but there are 3 failing tests to investigate

3152b35

Fix compilation errors with gcc

2b5b6a2

Fix R test failures

1e68145

Fix bugs inside otel macros

37008b8

Fix some bugs showing up with other compilers

f548fd7

Code review feedback

89ab44e

wesm force-pushed the ARROW-16757-scalar-output-modality branch from 1509ef8 to 89ab44e Compare July 7, 2022 20:19

[GLib] Follow API change

11e5f8f

wesm merged commit 6cc37cf into apache:master Jul 8, 2022

wesm deleted the ARROW-16757-scalar-output-modality branch July 8, 2022 02:07

nealrichardson mentioned this pull request Jul 8, 2022

ARROW-16268: [R] Remove long-deprecated functions #13550

Merged

wesm mentioned this pull request Jul 8, 2022

ARROW-16757: [C++][FOLLOWUP] Fix mingw32 RTools 4.0 build by removing usage of alignas #13557

Merged

asfimport mentioned this pull request Jul 13, 2022

[C++] Remove "scalar" output modality from array kernels #32092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class #13521

ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class #13521

wesm commented Jul 5, 2022 •

edited

Loading

github-actions bot commented Jul 5, 2022

wesm Jul 5, 2022

wesm Jul 5, 2022

wesm commented Jul 5, 2022

wesm commented Jul 6, 2022

kou commented Jul 6, 2022

lidavidm Jul 6, 2022

wesm commented Jul 6, 2022

wesm commented Jul 6, 2022

lidavidm commented Jul 6, 2022

kou commented Jul 7, 2022

kou commented Jul 7, 2022

kou commented Jul 7, 2022

pitrou left a comment

pitrou Jul 7, 2022

pitrou Jul 7, 2022

pitrou Jul 7, 2022

wesm commented Jul 7, 2022

kou commented Jul 8, 2022

wesm commented Jul 8, 2022

	Pretty-printer for arrow::UnionScalar and subclasses.
	Pretty-printer for arrow::SparseUnionScalar.

	Pretty-printer for arrow::UnionScalar and subclasses.
	Pretty-printer for arrow::DenseUnionScalar and subclasses.

ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class #13521

ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class #13521

Conversation

wesm commented Jul 5, 2022 • edited Loading

github-actions bot commented Jul 5, 2022

wesm Jul 5, 2022

Choose a reason for hiding this comment

wesm Jul 5, 2022

Choose a reason for hiding this comment

wesm commented Jul 5, 2022

wesm commented Jul 6, 2022

kou commented Jul 6, 2022

lidavidm Jul 6, 2022

Choose a reason for hiding this comment

wesm commented Jul 6, 2022

wesm commented Jul 6, 2022

lidavidm commented Jul 6, 2022

kou commented Jul 7, 2022

kou commented Jul 7, 2022

kou commented Jul 7, 2022

pitrou left a comment

Choose a reason for hiding this comment

pitrou Jul 7, 2022

Choose a reason for hiding this comment

pitrou Jul 7, 2022

Choose a reason for hiding this comment

pitrou Jul 7, 2022

Choose a reason for hiding this comment

wesm commented Jul 7, 2022

kou commented Jul 8, 2022

wesm commented Jul 8, 2022

wesm commented Jul 5, 2022 •

edited

Loading