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 2022/07/05 21:10:58 UTC
[GitHub] [arrow] wesm opened a new pull request, #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
wesm opened a new pull request, #13521:
URL: https://github.com/apache/arrow/pull/13521
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 Python and R bindings compile, but there are 3 failing R tests that I need to investigate
* The GLib bindings must be refactored to follow this work
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176953630
> I think that we need to add NumPy version to cache key.
#13534
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916198877
##########
cpp/src/arrow/array/data.h:
##########
@@ -266,16 +266,19 @@ struct ARROW_EXPORT ArraySpan {
int64_t offset = 0;
BufferSpan buffers[3];
+ // 16 bytes of scratch space to enable this ArraySpan to be a view onto
+ // scalar values including binary scalars (where we need to create a buffer
+ // that looks like two 32-bit or 64-bit offsets)
+ uint8_t scratch_space[16];
Review Comment:
Scalars that are a view onto a variable-size array (list, large list, binary, utf8, large binary, etc.) do not have an offsets buffer in their representation
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r915904390
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -304,10 +304,12 @@ class ARROW_EXPORT MapBuilder : public ArrayBuilder {
if (!validity || bit_util::GetBit(validity, array.offset + row)) {
ARROW_RETURN_NOT_OK(Append());
const int64_t slot_length = offsets[row + 1] - offsets[row];
+ // Add together the inner StructArray offset to the Map/List offset
+ int64_t key_value_offset = array.child_data[0].offset + offsets[row];
ARROW_RETURN_NOT_OK(key_builder_->AppendArraySlice(
- array.child_data[0].child_data[0], offsets[row], slot_length));
+ array.child_data[0].child_data[0], key_value_offset, slot_length));
ARROW_RETURN_NOT_OK(item_builder_->AppendArraySlice(
- array.child_data[0].child_data[1], offsets[row], slot_length));
+ array.child_data[0].child_data[1], key_value_offset, slot_length));
Review Comment:
Is it tested somewhere?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1175538170
@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
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916201356
##########
cpp/src/arrow/array/data.h:
##########
@@ -266,16 +266,19 @@ struct ARROW_EXPORT ArraySpan {
int64_t offset = 0;
BufferSpan buffers[3];
+ // 16 bytes of scratch space to enable this ArraySpan to be a view onto
+ // scalar values including binary scalars (where we need to create a buffer
+ // that looks like two 32-bit or 64-bit offsets)
+ uint8_t scratch_space[16];
Review Comment:
I'm making this aligned (by changing the type to be two uint64's)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r914208594
##########
cpp/src/arrow/array/data.h:
##########
@@ -266,16 +266,19 @@ struct ARROW_EXPORT ArraySpan {
int64_t offset = 0;
BufferSpan buffers[3];
+ // 16 bytes of scratch space to enable this ArraySpan to be a view onto
+ // scalar values including binary scalars (where we need to create a buffer
+ // that looks like two 32-bit or 64-bit offsets)
+ uint8_t scratch_space[16];
Review Comment:
It might make sense to put this "scratch space" only in the Scalar values that require it (types that have offsets), then we don't have to carry around an extra 16 bytes with every ArraySpan
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -385,13 +387,20 @@ int64_t ExecSpanIterator::GetNextChunkSpan(int64_t iteration_size, ExecSpan* spa
return iteration_size;
}
-bool ExecSpanIterator::Next(ExecSpan* span) {
- if (position_ == length_) {
- // This also protects from degenerate cases like ChunkedArrays
- // without any chunks
- return false;
+void PromoteExecSpanScalars(ExecSpan* span) {
Review Comment:
Here is the Scalar -> ArraySpan promotion
##########
cpp/src/arrow/compute/exec/test_util.cc:
##########
@@ -143,16 +143,25 @@ ExecNode* MakeDummyNode(ExecPlan* plan, std::string label, std::vector<ExecNode*
return node;
}
-ExecBatch ExecBatchFromJSON(const std::vector<ValueDescr>& descrs,
+ExecBatch ExecBatchFromJSON(const std::vector<TypeHolder>& types,
util::string_view json) {
auto fields = ::arrow::internal::MapVector(
- [](const ValueDescr& descr) { return field("", descr.type); }, descrs);
+ [](const TypeHolder& th) { return field("", th.GetSharedPtr()); }, types);
ExecBatch batch{*RecordBatchFromJSON(schema(std::move(fields)), json)};
+ return batch;
+}
+
+ExecBatch ExecBatchFromJSON(const std::vector<TypeHolder>& types,
+ const std::vector<ArgShape>& shapes, util::string_view json) {
Review Comment:
This is the only place that `ArgShape` is used now, just for the construction of test data
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -555,8 +505,7 @@ struct Kernel {
/// endeavor to write into pre-allocated memory if they are able, though for
/// some kernels (e.g. in cases when a builder like StringBuilder) must be
/// employed this may not be possible.
-using ArrayKernelExec =
- std::function<Status(KernelContext*, const ExecSpan&, ExecResult*)>;
+typedef Status (*ArrayKernelExec)(KernelContext*, const ExecSpan&, ExecResult*);
Review Comment:
Using plain function pointers makes debugging this code much easier (`std::function` adds many levels to gdb stack traces); we should use function pointers for the other kernel APIs in this file, too
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -143,10 +143,14 @@ ARROW_EXPORT std::shared_ptr<TypeMatcher> Primitive();
} // namespace match
-/// \brief An object used for type- and shape-checking arguments to be passed
-/// to a kernel and stored in a KernelSignature. Distinguishes between ARRAY
-/// and SCALAR arguments using ValueDescr::Shape. The type-checking rule can be
-/// supplied either with an exact DataType instance or a custom TypeMatcher.
+/// \brief Shape qualifier for value types. In certain instances
+/// (e.g. "map_lookup" kernel), an argument may only be a scalar, where in
+/// other kernels arguments can be arrays or scalars
+enum class ArgShape { ANY, ARRAY, SCALAR };
Review Comment:
This is ONLY used in testing. I will update the docstring and move this to the place where it's used
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -304,10 +304,12 @@ class ARROW_EXPORT MapBuilder : public ArrayBuilder {
if (!validity || bit_util::GetBit(validity, array.offset + row)) {
ARROW_RETURN_NOT_OK(Append());
const int64_t slot_length = offsets[row + 1] - offsets[row];
+ // Add together the inner StructArray offset to the Map/List offset
+ int64_t key_value_offset = array.child_data[0].offset + offsets[row];
ARROW_RETURN_NOT_OK(key_builder_->AppendArraySlice(
- array.child_data[0].child_data[0], offsets[row], slot_length));
+ array.child_data[0].child_data[0], key_value_offset, slot_length));
ARROW_RETURN_NOT_OK(item_builder_->AppendArraySlice(
- array.child_data[0].child_data[1], offsets[row], slot_length));
+ array.child_data[0].child_data[1], key_value_offset, slot_length));
Review Comment:
This fixes a bug that I introduced in a previous PR
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -254,19 +253,16 @@ inline bool operator==(const ExecBatch& l, const ExecBatch& r) { return l.Equals
inline bool operator!=(const ExecBatch& l, const ExecBatch& r) { return !l.Equals(r); }
struct ExecValue {
- enum Kind { ARRAY, SCALAR };
- Kind kind = ARRAY;
ArraySpan array;
- const Scalar* scalar;
+ const Scalar* scalar = NULLPTR;
Review Comment:
I applied Antoine's suggested optimization
##########
cpp/src/arrow/compute/function_benchmark.cc:
##########
@@ -19,6 +19,7 @@
#include "arrow/array/array_base.h"
Review Comment:
Need to check this benchmark wasn't broken
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -235,12 +235,11 @@ struct ARROW_EXPORT ExecBatch {
ExecBatch Slice(int64_t offset, int64_t length) const;
- /// \brief A convenience for returning the ValueDescr objects (types and
- /// shapes) from the batch.
- std::vector<ValueDescr> GetDescriptors() const {
- std::vector<ValueDescr> result;
+ /// \brief A convenience for returning the types from the batch.
+ std::vector<TypeHolder> GetTypes() const {
+ std::vector<TypeHolder> result;
for (const auto& value : this->values) {
- result.emplace_back(value.descr());
+ result.emplace_back(value.type());
Review Comment:
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)
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -324,29 +310,21 @@ struct ExecValue {
struct ARROW_EXPORT ExecResult {
// The default value of the variant is ArraySpan
- // TODO(wesm): remove Scalar output modality in ARROW-16577
- util::Variant<ArraySpan, std::shared_ptr<ArrayData>, std::shared_ptr<Scalar>> value;
+ util::Variant<ArraySpan, std::shared_ptr<ArrayData>> value;
Review Comment:
Kernels can no longer return `shared_ptr<Scalar>`
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -430,6 +434,12 @@ static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromSca
return value->type;
}
+template <typename T>
+static inline enable_if_same_result<T, TypeHolder> GenericFromScalar(
+ const std::shared_ptr<Scalar>& value) {
+ return value->type;
+}
+
Review Comment:
Aside: I struggled a bit with this reflection stuff
##########
cpp/src/arrow/compute/kernels/vector_cumulative_ops_test.cc:
##########
@@ -66,21 +66,45 @@ TEST(TestCumulativeSum, AllNulls) {
}
}
-using testing::HasSubstr;
-
-TEST(TestCumulativeSum, ScalarNotSupported) {
- CumulativeSumOptions options;
-
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- NotImplemented, HasSubstr("no kernel"),
- CallFunction("cumulative_sum", {std::make_shared<Int64Scalar>(5)}, &options));
+TEST(TestCumulativeSum, ScalarInput) {
+ CumulativeSumOptions no_start_no_skip;
+ CumulativeSumOptions no_start_do_skip(0, true);
+ CumulativeSumOptions has_start_no_skip(10);
+ CumulativeSumOptions has_start_do_skip(10, true);
- EXPECT_RAISES_WITH_MESSAGE_THAT(
- NotImplemented, HasSubstr("no kernel"),
- CallFunction("cumulative_sum_checked", {std::make_shared<Int64Scalar>(5)},
- &options));
+ for (auto ty : NumericTypes()) {
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "10"),
+ ArrayFromJSON(ty, "[10]"), &no_start_no_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "10"),
+ ArrayFromJSON(ty, "[10]"), &no_start_no_skip);
+
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "10"),
+ ArrayFromJSON(ty, "[20]"), &has_start_no_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "10"),
+ ArrayFromJSON(ty, "[20]"), &has_start_no_skip);
+
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &no_start_no_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &no_start_no_skip);
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &has_start_no_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &has_start_no_skip);
+
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &no_start_do_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &no_start_do_skip);
+ CheckVectorUnary("cumulative_sum", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &has_start_do_skip);
+ CheckVectorUnary("cumulative_sum_checked", ScalarFromJSON(ty, "null"),
+ ArrayFromJSON(ty, "[null]"), &has_start_do_skip);
+ }
Review Comment:
These tests are restored from having been deleted in one of my recent PRs
##########
cpp/src/arrow/scalar.h:
##########
@@ -110,7 +108,7 @@ struct ARROW_EXPORT Scalar : public std::enable_shared_from_this<Scalar>,
/// \brief EXPERIMENTAL Enable obtaining shared_ptr<Scalar> from a const
/// Scalar& context. Implementation depends on enable_shared_from_this, but
/// we may change this in the future
- std::shared_ptr<Scalar> Copy() const {
+ std::shared_ptr<Scalar> GetSharedPtr() const {
Review Comment:
Per previous comments
##########
cpp/src/arrow/type_fwd.h:
##########
@@ -416,43 +416,43 @@ struct Type {
/// @{
/// \brief Return a NullType instance
-std::shared_ptr<DataType> ARROW_EXPORT null();
+const std::shared_ptr<DataType>& ARROW_EXPORT null();
Review Comment:
This way we can do `null().get()` or `int32().get()` to get a pointer without having to copy the shared_ptr every time we call these functions (they return static instances)
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +481,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
Review Comment:
Note: this change is necessary to establish a 1-1 mapping between SparseUnionScalar and an ArraySpan referencing a single-element slice of a SparseUnionArray. It's annoying but I think it's the simplest thing
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1175502364
https://issues.apache.org/jira/browse/ARROW-16757
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm merged pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm merged PR #13521:
URL: https://github.com/apache/arrow/pull/13521
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r915974431
##########
cpp/src/arrow/compute/function_benchmark.cc:
##########
@@ -85,15 +85,13 @@ void BM_CastDispatchBaseline(benchmark::State& state) {
.ValueOrDie();
kernel_context.SetState(cast_state.get());
- ExecSpan input;
- input.length = 1;
+ ExecSpan input({ExecValue(*int_array->data())}, 1);
+ ExecResult result;
+ ASSERT_OK_AND_ASSIGN(std::shared_ptr<Array> result_space,
+ MakeArrayOfNull(double_type, 1));
+ result.array_span()->SetMembers(*result_space->data());
for (auto _ : state) {
- ExecResult result;
- result.value = MakeNullScalar(double_type);
- for (const std::shared_ptr<Scalar>& int_scalar : int_scalars) {
- input.values = {ExecValue(int_scalar.get())};
- ABORT_NOT_OK(exec(&kernel_context, input, &result));
- }
+ ABORT_NOT_OK(exec(&kernel_context, input, &result));
Review Comment:
Hmm, the point here was to execute on bunch of different scalars, not a single one. I'm not sure it makes much of a difference in practice, except that `setItemsProcessed` below is now wrong.
##########
cpp/src/arrow/compute/kernels/scalar_nested_test.cc:
##########
@@ -603,11 +603,17 @@ TEST(MakeStruct, Scalar) {
EXPECT_THAT(MakeStructor({i32, f64, str}),
ResultWith(Datum(*StructScalar::Make({i32, f64, str}, {"0", "1", "2"}))));
- // No field names or input values is fine
- EXPECT_THAT(MakeStructor({}), ResultWith(Datum(*StructScalar::Make({}, {}))));
-
// Three field names but one input value
EXPECT_THAT(MakeStructor({str}, {"i", "f", "s"}), Raises(StatusCode::Invalid));
+
+ // ARROW-16757: No input values yields empty struct array of length 1
+ ScalarVector value;
+ auto empty_scalar = std::make_shared<StructScalar>(value, struct_({}));
+ ASSERT_OK_AND_ASSIGN(std::shared_ptr<Array> empty_result,
+ MakeArrayFromScalar(*empty_scalar, 0));
Review Comment:
If passing length = 0 is deliberate here, then perhaps you can simply use `MakeEmptyArray`?
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1648,17 +1721,20 @@ TEST_F(TestExtensionScalar, ValidateErrors) {
AssertValidationFails(uuid_scalar);
// Null storage scalar
- auto null_storage = std::make_shared<FixedSizeBinaryScalar>(storage_type_);
+ auto null_storage = MakeNullScalar(storage_type_);
ExtensionScalar scalar(null_storage, type_);
scalar.is_valid = true;
AssertValidationFails(scalar);
+
+ // If the scalar is null it's okay
scalar.is_valid = false;
- AssertValidationFails(scalar);
+ ASSERT_OK(scalar.ValidateFull());
// Invalid storage scalar (wrong length)
- auto invalid_storage = std::make_shared<FixedSizeBinaryScalar>(storage_type_);
+ std::shared_ptr<Scalar> invalid_storage = MakeNullScalar(storage_type_);
invalid_storage->is_valid = true;
- invalid_storage->value = std::make_shared<Buffer>("123");
+ static_cast<FixedSizeBinaryScalar*>(invalid_storage.get())->value =
Review Comment:
`checked_cast`
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -784,30 +811,28 @@ class ScalarExecutor : public KernelExecutorImpl<ScalarKernel> {
Datum WrapResults(const std::vector<Datum>& inputs,
const std::vector<Datum>& outputs) override {
- if (output_descr_.shape == ValueDescr::SCALAR) {
- // TODO(wesm): to remove, see ARROW-16757
- DCHECK_EQ(outputs.size(), 1);
- // Return as SCALAR
- return outputs[0];
+ // If execution yielded multiple chunks (because large arrays were split
+ // based on the ExecContext parameters, then the result is a ChunkedArray
+ if (HaveChunkedArray(inputs) || outputs.size() > 1) {
+ return ToChunkedArray(outputs, output_type_);
} else {
- // If execution yielded multiple chunks (because large arrays were split
- // based on the ExecContext parameters, then the result is a ChunkedArray
- if (HaveChunkedArray(inputs) || outputs.size() > 1) {
- return ToChunkedArray(outputs, output_descr_.type);
- } else if (outputs.size() == 1) {
- // Outputs have just one element
- return outputs[0];
- } else {
- // XXX: In the case where no outputs are omitted, is returning a 0-length
- // array always the correct move?
- return MakeArrayOfNull(output_descr_.type, /*length=*/0,
- exec_context()->memory_pool())
- .ValueOrDie();
- }
+ // Outputs have just one element
+ return outputs[0];
}
}
protected:
+ Status EmitResult(std::shared_ptr<ArrayData> out, ExecListener* listener) {
Review Comment:
`out` could be passed by const-ref here?
##########
cpp/src/arrow/compute/exec_test.cc:
##########
@@ -1154,8 +1155,9 @@ TEST_F(TestCallScalarFunction, ArgumentValidation) {
ASSERT_RAISES(Invalid, CallFunction("test_copy", args));
// Cannot do scalar
- args = {Datum(std::make_shared<Int32Scalar>(5))};
- ASSERT_RAISES(NotImplemented, CallFunction("test_copy", args));
+ Datum d1_scalar(std::make_shared<Int32Scalar>(5));
+ ASSERT_OK_AND_ASSIGN(auto result, CallFunction("test_copy", {d1}));
+ ASSERT_OK_AND_ASSIGN(result, CallFunction("test_copy", {d1_scalar}));
Review Comment:
We should probably test the result values, to check that the Scalar->ArraySpan->Scalar plumbing works.
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -122,11 +124,11 @@ class TimeUnitMatcher : public TypeMatcher {
explicit TimeUnitMatcher(TimeUnit::type accepted_unit)
: accepted_unit_(accepted_unit) {}
- bool Matches(const DataType& type) const override {
+ bool Matches(const TypeHolder& type) const override {
Review Comment:
Same here, and below as well.
##########
cpp/src/arrow/compute/kernels/scalar_if_else_test.cc:
##########
@@ -1215,19 +1220,28 @@ TYPED_TEST(TestCaseWhenNumeric, FixedSize) {
// Error cases
EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, ::testing::HasSubstr("cond struct must not be null"),
- CallFunction(
- "case_when",
- {Datum(std::make_shared<StructScalar>(struct_({field("", boolean())}))),
- Datum(scalar1)}));
+ Invalid,
+ ::testing::HasSubstr("cond struct must not be a null scalar or "
+ "have top-level nulls"),
+ CallFunction("case_when",
+ {MakeNullScalar(struct_({field("", boolean())})), Datum(scalar1)}));
EXPECT_RAISES_WITH_MESSAGE_THAT(
- Invalid, ::testing::HasSubstr("cond struct must not have top-level nulls"),
+ Invalid,
+ ::testing::HasSubstr("cond struct must not be a null scalar or "
+ "have top-level nulls"),
CallFunction("case_when",
{Datum(*MakeArrayOfNull(struct_({field("", boolean())}), 4)),
Datum(values1)}));
}
}
+template <typename Type>
+class TestCaseWhenNumeric : public ::testing::Test {};
+
+TYPED_TEST_SUITE(TestCaseWhenNumeric, IfElseNumericBasedTypes);
+
+TYPED_TEST(TestCaseWhenNumeric, FixedSize) { TestCaseWhenFixedSize<TypeParam>(); }
Review Comment:
Hmm, is there any reason for the `TestCaseWhenFixedSize` redirection? It probably makes reading the test a bit more cumbersome.
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
Review Comment:
`BufferSpan` has all fields zero-initialized, so are such explicit zero-initializations required?
##########
cpp/src/arrow/compute/kernels/scalar_nested_test.cc:
##########
@@ -603,11 +603,17 @@ TEST(MakeStruct, Scalar) {
EXPECT_THAT(MakeStructor({i32, f64, str}),
ResultWith(Datum(*StructScalar::Make({i32, f64, str}, {"0", "1", "2"}))));
- // No field names or input values is fine
- EXPECT_THAT(MakeStructor({}), ResultWith(Datum(*StructScalar::Make({}, {}))));
-
// Three field names but one input value
EXPECT_THAT(MakeStructor({str}, {"i", "f", "s"}), Raises(StatusCode::Invalid));
+
+ // ARROW-16757: No input values yields empty struct array of length 1
Review Comment:
Length 0 is passed to `MakeArrayFromScalar`, is there something wrong?
##########
cpp/src/arrow/python/udf.cc:
##########
@@ -27,18 +27,15 @@ using compute::ExecSpan;
namespace py {
namespace {
-Status CheckOutputType(const DataType& expected, const DataType& actual) {
- if (!expected.Equals(actual)) {
- return Status::TypeError("Expected output datatype ", expected.ToString(),
- ", but function returned datatype ", actual.ToString());
- }
- return Status::OK();
-}
-struct PythonUdf {
+struct PythonUdf : public compute::KernelState {
ScalarUdfWrapperCallback cb;
std::shared_ptr<OwnedRefNoGIL> function;
- compute::OutputType output_type;
+ std::shared_ptr<DataType> output_type;
+
+ PythonUdf(ScalarUdfWrapperCallback cb, std::shared_ptr<OwnedRefNoGIL> function,
+ const std::shared_ptr<DataType>& output_type)
+ : cb(cb), function(function), output_type(output_type) {}
Review Comment:
```suggestion
: cb(std::move(cb)), function(std::move(function)), output_type(output_type) {}
```
##########
cpp/src/arrow/compute/exec.h:
##########
@@ -235,12 +235,11 @@ struct ARROW_EXPORT ExecBatch {
ExecBatch Slice(int64_t offset, int64_t length) const;
- /// \brief A convenience for returning the ValueDescr objects (types and
- /// shapes) from the batch.
- std::vector<ValueDescr> GetDescriptors() const {
- std::vector<ValueDescr> result;
+ /// \brief A convenience for returning the types from the batch.
+ std::vector<TypeHolder> GetTypes() const {
+ std::vector<TypeHolder> result;
Review Comment:
Might want to presize the array to avoid spurious upsizes.
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -661,14 +601,17 @@ struct FixedSizeBinaryScalarMinMax {
const auto data = array.GetValues<uint8_t>(1, /*absolute_offset=*/0);
visit_value(string_view(
reinterpret_cast<const char*>(data) + row * byte_width, byte_width));
+ num_valid_values += 1;
} else if (!options.skip_nulls) {
- result = string_view();
+ // If we encounter a null, exit the loop and mark num_row_values to
Review Comment:
Same question here re: num_row_values
##########
cpp/src/arrow/compute/cast.cc:
##########
@@ -213,36 +201,48 @@ Result<const Kernel*> CastFunction::DispatchExact(
return candidate_kernels[0];
}
+Result<std::shared_ptr<CastFunction>> GetCastFunction(const TypeHolder& to_type) {
+ return internal::GetCastFunctionInternal(to_type);
Review Comment:
So there's `internal::GetCastFunction` which redirects to `internal::GetCastFunctionInternal`? Can this perhaps be simplified?
##########
cpp/src/arrow/compute/exec_test.cc:
##########
@@ -1154,8 +1155,9 @@ TEST_F(TestCallScalarFunction, ArgumentValidation) {
ASSERT_RAISES(Invalid, CallFunction("test_copy", args));
// Cannot do scalar
Review Comment:
Should this comment be removed since it now seems to be supported?
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -227,21 +208,16 @@ class ARROW_EXPORT InputType {
/// \brief Render a human-readable string representation.
std::string ToString() const;
- /// \brief Return true if the value matches this argument kind in type
- /// and shape.
+ /// \brief Return true if the Datum matches this argument kind in
+ /// type (and only allows scalar or array-like Datums).
bool Matches(const Datum& value) const;
- /// \brief Return true if the value descriptor matches this argument kind in
- /// type and shape.
- bool Matches(const ValueDescr& value) const;
+ /// \brief Return true if the type matches this InputType
+ bool Matches(const TypeHolder& type) const;
Review Comment:
Same here: why not `const DataType&`? I suppose this isn't keeping a copy of the `TypeHolder`?
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
Review Comment:
Make these helpers static/put them in the anonymous namespace?
##########
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##########
@@ -323,14 +318,31 @@ BinaryToBinaryCastExec(KernelContext* ctx, const ExecSpan& batch, ExecResult* ou
arrow::internal::CopyBitmap(ctx->memory_pool(), input.buffers[0].data,
input.offset, input.length));
}
- // Data buffer (index 1) for FWBinary becomes data buffer for
- // VarBinary (index 2)
- output->buffers[2] = input.GetBuffer(1);
+
+ // This buffer is preallocated
output_offset_type* offsets = output->GetMutableValues<output_offset_type>(1);
offsets[0] = static_cast<output_offset_type>(input.offset * width);
for (int64_t i = 0; i < input.length; i++) {
offsets[i + 1] = offsets[i] + width;
}
+
+ // Data buffer (index 1) for FWBinary becomes data buffer for VarBinary
+ // (index 2). After ARROW-16757, we need to copy this memory instead of
+ // zero-copy it because a Scalar value promoted to an ArraySpan may be
+ // referencing a temporary buffer whose scope does not extend beyond the
+ // kernel execution. In that scenario, the validity bitmap above can be
Review Comment:
This is a bit of a pity. Perhaps we should open a JIRA to try and improve this?
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -186,87 +170,106 @@ const Kernel* DispatchExactImpl(const Function* func,
} // namespace detail
Result<const Kernel*> Function::DispatchExact(
- const std::vector<ValueDescr>& values) const {
+ const std::vector<TypeHolder>& values) const {
if (kind_ == Function::META) {
return Status::NotImplemented("Dispatch for a MetaFunction's Kernels");
}
- RETURN_NOT_OK(CheckArity(values));
+ RETURN_NOT_OK(CheckArity(values.size()));
if (auto kernel = detail::DispatchExactImpl(this, values)) {
return kernel;
}
return detail::NoMatchingKernel(this, values);
}
-Result<const Kernel*> Function::DispatchBest(std::vector<ValueDescr>* values) const {
+Result<const Kernel*> Function::DispatchBest(std::vector<TypeHolder>* values) const {
// TODO(ARROW-11508) permit generic conversions here
return DispatchExact(*values);
}
-Result<Datum> Function::Execute(const std::vector<Datum>& args,
- const FunctionOptions* options, ExecContext* ctx) const {
- return ExecuteInternal(args, /*passed_length=*/-1, options, ctx);
+namespace {
+
+/// \brief Check that each Datum is of a "value" type, which means either
+/// SCALAR, ARRAY, or CHUNKED_ARRAY.
+Status CheckAllValues(const std::vector<Datum>& values) {
+ for (const auto& value : values) {
+ if (!value.is_value()) {
+ return Status::Invalid("Tried executing function with non-value type: ",
+ value.ToString());
+ }
+ }
+ return Status::OK();
}
-Result<Datum> Function::Execute(const ExecBatch& batch, const FunctionOptions* options,
- ExecContext* ctx) const {
- return ExecuteInternal(batch.values, batch.length, options, ctx);
+Status CheckOptions(const Function& function, const FunctionOptions* options) {
+ if (options == nullptr && function.doc().options_required) {
+ return Status::Invalid("Function '", function.name(),
+ "' cannot be called without options");
+ }
+ return Status::OK();
}
-Result<Datum> Function::ExecuteInternal(const std::vector<Datum>& args,
- int64_t passed_length,
- const FunctionOptions* options,
- ExecContext* ctx) const {
+Result<Datum> ExecuteInternal(const Function& func, std::vector<Datum> args,
Review Comment:
For the record, if you want to further micro-optimize function execution, you might want to migrate `args` to `SmallVector`, to avoid dynamic allocation. Or perhaps that's a bit overkill.
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
Review Comment:
```suggestion
memset(span->scratch_space, 0x00, sizeof(span->scratch_space));
```
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
+ }
+ } else if (type_id == Type::STRUCT) {
+ const auto& scalar = checked_cast<const StructScalar&>(value);
+ this->child_data.resize(this->type->num_fields());
+ DCHECK_EQ(this->type->num_fields(), static_cast<int>(scalar.value.size()));
+ for (size_t i = 0; i < scalar.value.size(); ++i) {
+ this->child_data[i].FillFromScalar(*scalar.value[i]);
+ }
+ } else if (is_union(type_id)) {
+ // First buffer is kept null since unions have no validity vector
+ this->buffers[0].data = nullptr;
+ this->buffers[0].size = 0;
+
+ this->buffers[1].data = this->scratch_space;
+ this->buffers[1].size = 1;
+ int8_t* type_codes = reinterpret_cast<int8_t*>(this->scratch_space);
+ type_codes[0] = checked_cast<const UnionScalar&>(value).type_code;
+
+ this->child_data.resize(this->type->num_fields());
+ if (type_id == Type::DENSE_UNION) {
+ const auto& scalar = checked_cast<const DenseUnionScalar&>(value);
+ // Has offset; start 4 bytes in so it's aligned to a 32-bit boundaries
Review Comment:
What does adding 4 change to 4-alignment?
##########
cpp/src/arrow/compute/kernels/scalar_compare.cc:
##########
@@ -645,13 +581,17 @@ struct FixedSizeBinaryScalarMinMax {
result = result.empty() ? value : Op::Call(result, value);
};
+ int num_valid_values = 0;
for (int col = 0; col < batch.num_values(); col++) {
if (batch[col].is_scalar()) {
const Scalar& scalar = *batch[col].scalar;
if (scalar.is_valid) {
visit_value(UnboxScalar<FixedSizeBinaryType>::Unbox(scalar));
+ num_valid_values += 1;
} else if (!options.skip_nulls) {
- result = string_view();
+ // If we encounter a null, exit the loop and mark num_row_values to
Review Comment:
Hmm, I don't see a num_row_values?
##########
cpp/src/arrow/compute/function_benchmark.cc:
##########
@@ -153,31 +151,26 @@ void BM_ExecuteScalarFunctionOnScalar(benchmark::State& state) {
void BM_ExecuteScalarKernelOnScalar(benchmark::State& state) {
// Execute a trivial function, with argument dispatch outside the hot path
- const int64_t N = 10000;
-
auto function = *GetFunctionRegistry()->GetFunction("is_valid");
- auto kernel = *function->DispatchExact({ValueDescr::Scalar(int64())});
+ auto kernel = *function->DispatchExact({int64()});
const auto& exec = static_cast<const ScalarKernel&>(*kernel).exec;
- const auto scalars = MakeScalarsForIsValid(N);
-
ExecContext exec_context;
KernelContext kernel_context(&exec_context);
- ExecSpan input;
- input.length = 1;
+ ASSERT_OK_AND_ASSIGN(std::shared_ptr<Array> input_arr, MakeArrayOfNull(int64(), 1));
+ ExecSpan input({*input_arr->data()}, 1);
+
+ ExecResult output;
+ ASSERT_OK_AND_ASSIGN(std::shared_ptr<Array> output_arr, MakeArrayOfNull(int64(), 1));
+ output.array_span()->SetMembers(*output_arr->data());
+
+ const int64_t N = 10000;
for (auto _ : state) {
- int64_t total = 0;
- for (const std::shared_ptr<Scalar>& scalar : scalars) {
- ExecResult result;
- result.value = MakeNullScalar(int64());
- input.values = {scalar.get()};
- ABORT_NOT_OK(exec(&kernel_context, input, &result));
- total += result.scalar()->is_valid;
+ for (int i = 0; i < N; ++i) {
+ ABORT_NOT_OK(exec(&kernel_context, input, &output));
}
- benchmark::DoNotOptimize(total);
Review Comment:
This was meant to ensure that the compiler didn't start to optimize the computation away. Not sure whether it's actually necessary but probably good to keep?
##########
cpp/src/arrow/python/udf.cc:
##########
@@ -73,34 +66,29 @@ struct PythonUdf {
OwnedRef result(cb(function->obj(), udf_context, arg_tuple.obj()));
RETURN_NOT_OK(CheckPyError());
// unwrapping the output for expected output type
- if (is_scalar(result.obj())) {
- if (out->is_array_data()) {
- return Status::TypeError(
- "UDF executor expected an array result but a "
- "scalar was returned");
- }
- ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Scalar> val, unwrap_scalar(result.obj()));
- RETURN_NOT_OK(CheckOutputType(*output_type.type(), *val->type));
- out->value = val;
- return Status::OK();
- } else if (is_array(result.obj())) {
- if (out->is_scalar()) {
- return Status::TypeError(
- "UDF executor expected a scalar result but an "
- "array was returned");
- }
+ if (is_array(result.obj())) {
ARROW_ASSIGN_OR_RAISE(std::shared_ptr<Array> val, unwrap_array(result.obj()));
- RETURN_NOT_OK(CheckOutputType(*output_type.type(), *val->type()));
+ if (!output_type->Equals(*val->type())) {
+ return Status::TypeError("Expected output datatype ", output_type->ToString(),
+ ", but function returned datatype ",
+ val->type()->ToString());
+ }
out->value = std::move(val->data());
return Status::OK();
} else {
return Status::TypeError("Unexpected output type: ", Py_TYPE(result.obj())->tp_name,
- " (expected Scalar or Array)");
+ " (expected Array)");
}
return Status::OK();
}
};
+Status PythonUdfExec(compute::KernelContext* ctx, const ExecSpan& batch,
+ ExecResult* out) {
+ auto udf = static_cast<PythonUdf*>(ctx->kernel()->data.get());
Review Comment:
Is it possible to use `checked_cast` here?
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -275,9 +278,9 @@ Result<Datum> Function::ExecuteInternal(const std::vector<Datum>& args,
bool all_same_length = false;
int64_t inferred_length = detail::InferBatchLength(input.values, &all_same_length);
input.length = inferred_length;
- if (kind() == Function::SCALAR) {
+ if (func.kind() == Function::SCALAR) {
DCHECK(passed_length == -1 || passed_length == inferred_length);
Review Comment:
Code didn't change in this PR, but could we change this `DCHECK` into a proper error return?
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +480,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
+
+ // The value index corresponding to the active type code
+ int child_id;
Review Comment:
Can we also have a `const std::shared_ptr<Scalar>& child_value() const` accessor to ease the work of SparseUnionScalar consumers?
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +480,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
+
+ // The value index corresponding to the active type code
+ int child_id;
+
+ SparseUnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type);
+
+ /// \brief Construct a SparseUnionScalar from a single value, versus having
+ /// to construct a vector of scalars
+ static std::shared_ptr<Scalar> FromValue(std::shared_ptr<Scalar> value, int field_index,
+ std::shared_ptr<DataType> type);
};
struct ARROW_EXPORT DenseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = DenseUnionType;
+
+ // For DenseUnionScalar, we can make a valid ArraySpan of length 1 from this
+ // scalar
+ using ValueType = std::shared_ptr<Scalar>;
+ ValueType value;
Review Comment:
If we add `const std::shared_ptr<Scalar>& child_value() const` to SparseUnionScalar, can we also add it here for consistency and to allow more code reuse at call sites?
##########
cpp/src/arrow/scalar.cc:
##########
@@ -703,7 +792,11 @@ std::string Scalar::ToString() const {
if (maybe_repr.ok()) {
return checked_cast<const StringScalar&>(*maybe_repr.ValueOrDie()).value->ToString();
}
- return "...";
+
+ std::string result;
Review Comment:
Should we instead open a JIRA if the scalar cast is not implemented for some types?
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1370,27 +1373,61 @@ TEST(TestDictionaryScalar, Cast) {
}
}
+const Scalar& GetUnionValue(const Scalar& value) {
+ if (value.type->id() == Type::DENSE_UNION) {
+ return *checked_cast<const DenseUnionScalar&>(value).value;
+ } else {
+ const auto& union_scalar = checked_cast<const SparseUnionScalar&>(value);
+ return *union_scalar.value[union_scalar.child_id];
+ }
+}
+
void CheckGetValidUnionScalar(const Array& arr, int64_t index, const Scalar& expected,
const Scalar& expected_value) {
ASSERT_OK_AND_ASSIGN(auto scalar, arr.GetScalar(index));
ASSERT_OK(scalar->ValidateFull());
ASSERT_TRUE(scalar->Equals(expected));
- const auto& as_union = checked_cast<const UnionScalar&>(*scalar);
- ASSERT_TRUE(as_union.is_valid);
- ASSERT_TRUE(as_union.value->Equals(expected_value));
+ ASSERT_TRUE(scalar->is_valid);
+ ASSERT_TRUE(GetUnionValue(*scalar).Equals(expected_value));
}
void CheckGetNullUnionScalar(const Array& arr, int64_t index) {
ASSERT_OK_AND_ASSIGN(auto scalar, arr.GetScalar(index));
ASSERT_TRUE(scalar->Equals(MakeNullScalar(arr.type())));
- const auto& as_union = checked_cast<const UnionScalar&>(*scalar);
- ASSERT_FALSE(as_union.is_valid);
- // XXX in reality, the union array doesn't have a validity bitmap.
- // Validity is inferred from the underlying child value, which should maybe
- // be reflected here...
- ASSERT_EQ(as_union.value, nullptr);
+ ASSERT_FALSE(scalar->is_valid);
+ ASSERT_FALSE(GetUnionValue(*scalar).is_valid);
+}
+
+std::shared_ptr<Scalar> MakeUnionScalar(const SparseUnionType& type,
Review Comment:
These four specific helpers might be useful enough to be made static methods of `{Dense,Sparse}UnionScalar` ?
##########
cpp/src/arrow/type.h:
##########
@@ -210,9 +210,68 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this<DataType>,
ARROW_DISALLOW_COPY_AND_ASSIGN(DataType);
};
+/// \brief EXPERIMENTAL: Container for a type pointer which can hold a
+/// dynamically created shared_ptr<DataType> if it needs to.
+struct ARROW_EXPORT TypeHolder {
+ const DataType* type = NULLPTR;
+ std::shared_ptr<DataType> owned_type;
+
+ TypeHolder() = default;
+ TypeHolder(const TypeHolder& other) = default;
+ TypeHolder& operator=(const TypeHolder& other) = default;
+ TypeHolder(TypeHolder&& other) = default;
+ TypeHolder& operator=(TypeHolder&& other) = default;
+
+ TypeHolder(std::shared_ptr<DataType> owned_type) // NOLINT implicit construction
+ : type(owned_type.get()), owned_type(std::move(owned_type)) {}
+
+ TypeHolder(const DataType* type) // NOLINT implicit construction
+ : type(type) {}
+
+ Type::type id() const { return this->type->id(); }
+
+ std::shared_ptr<DataType> GetSharedPtr() const {
+ return this->type != NULLPTR ? this->type->GetSharedPtr() : NULLPTR;
+ }
+
+ const DataType& operator*() const { return *this->type; }
+
+ operator bool() { return this->type != NULLPTR; }
+
+ bool operator==(const TypeHolder& other) const {
+ if (type == other.type) return true;
+ if (type == NULLPTR || other.type == NULLPTR) return false;
+ return type->Equals(*other.type);
+ }
+
+ bool operator==(decltype(NULLPTR)) const { return this->type == NULLPTR; }
+
+ bool operator==(const DataType& other) const {
+ if (this->type == NULLPTR) return false;
+ return other.Equals(*this->type);
+ }
+
+ bool operator!=(const DataType& other) const { return !(*this == other); }
+
+ bool operator==(const std::shared_ptr<DataType>& other) const {
Review Comment:
Neither does this one?
##########
cpp/src/arrow/type.h:
##########
@@ -210,9 +210,68 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this<DataType>,
ARROW_DISALLOW_COPY_AND_ASSIGN(DataType);
};
+/// \brief EXPERIMENTAL: Container for a type pointer which can hold a
+/// dynamically created shared_ptr<DataType> if it needs to.
+struct ARROW_EXPORT TypeHolder {
+ const DataType* type = NULLPTR;
+ std::shared_ptr<DataType> owned_type;
+
+ TypeHolder() = default;
+ TypeHolder(const TypeHolder& other) = default;
+ TypeHolder& operator=(const TypeHolder& other) = default;
+ TypeHolder(TypeHolder&& other) = default;
+ TypeHolder& operator=(TypeHolder&& other) = default;
+
+ TypeHolder(std::shared_ptr<DataType> owned_type) // NOLINT implicit construction
+ : type(owned_type.get()), owned_type(std::move(owned_type)) {}
+
+ TypeHolder(const DataType* type) // NOLINT implicit construction
+ : type(type) {}
+
+ Type::type id() const { return this->type->id(); }
+
+ std::shared_ptr<DataType> GetSharedPtr() const {
+ return this->type != NULLPTR ? this->type->GetSharedPtr() : NULLPTR;
+ }
+
+ const DataType& operator*() const { return *this->type; }
+
+ operator bool() { return this->type != NULLPTR; }
+
+ bool operator==(const TypeHolder& other) const {
+ if (type == other.type) return true;
+ if (type == NULLPTR || other.type == NULLPTR) return false;
+ return type->Equals(*other.type);
+ }
+
+ bool operator==(decltype(NULLPTR)) const { return this->type == NULLPTR; }
Review Comment:
This one has no corresponding `operator!=`?
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -87,7 +87,9 @@ class SameTypeIdMatcher : public TypeMatcher {
public:
explicit SameTypeIdMatcher(Type::type accepted_id) : accepted_id_(accepted_id) {}
- bool Matches(const DataType& type) const override { return type.id() == accepted_id_; }
+ bool Matches(const TypeHolder& type) const override {
Review Comment:
Is there a reason to change this? `type` is not stored and it's probably a bit more efficient to simply pass `const DataType&`.
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
Review Comment:
Even for `Type::NA`?
##########
python/pyarrow/scalar.pxi:
##########
@@ -931,12 +938,15 @@ cdef class ExtensionScalar(Scalar):
else:
storage = scalar(value, typ.storage_type)
- sp_scalar = make_shared[CExtensionScalar](typ.sp_type)
- ext_scalar = sp_scalar.get()
- ext_scalar.is_valid = storage is not None and storage.is_valid
- if ext_scalar.is_valid:
- ext_scalar.value = pyarrow_unwrap_scalar(storage)
- check_status(ext_scalar.Validate())
+ cdef c_bool is_valid = storage is not None and storage.is_valid
+ if is_valid:
+ sp_storage = pyarrow_unwrap_scalar(storage)
+ else:
+ sp_storage = MakeNullScalar((<DataType> typ.storage_type).sp_type)
Review Comment:
Hmm, if `storage is not None`, then we should also use `pyarrow_unwrap_scalar(storage)`, no?
##########
cpp/gdb_arrow.py:
##########
@@ -1463,7 +1464,24 @@ def to_string(self):
return f"{self._format_type()}"
-class UnionScalarPrinter(ScalarPrinter):
+class SparseUnionScalarPrinter(ScalarPrinter):
+ """
+ Pretty-printer for arrow::UnionScalar and subclasses.
Review Comment:
```suggestion
Pretty-printer for arrow::SparseUnionScalar.
```
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
+ }
+ } else if (type_id == Type::STRUCT) {
+ const auto& scalar = checked_cast<const StructScalar&>(value);
+ this->child_data.resize(this->type->num_fields());
+ DCHECK_EQ(this->type->num_fields(), static_cast<int>(scalar.value.size()));
+ for (size_t i = 0; i < scalar.value.size(); ++i) {
+ this->child_data[i].FillFromScalar(*scalar.value[i]);
+ }
+ } else if (is_union(type_id)) {
+ // First buffer is kept null since unions have no validity vector
+ this->buffers[0].data = nullptr;
+ this->buffers[0].size = 0;
+
+ this->buffers[1].data = this->scratch_space;
+ this->buffers[1].size = 1;
+ int8_t* type_codes = reinterpret_cast<int8_t*>(this->scratch_space);
+ type_codes[0] = checked_cast<const UnionScalar&>(value).type_code;
+
+ this->child_data.resize(this->type->num_fields());
+ if (type_id == Type::DENSE_UNION) {
+ const auto& scalar = checked_cast<const DenseUnionScalar&>(value);
+ // Has offset; start 4 bytes in so it's aligned to a 32-bit boundaries
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space + sizeof(int32_t), 1,
+ /*buffer_index=*/2);
+ // We can't "see" the other arrays in the union, but we put the "active"
+ // union array in the right place and fill zero-length arrays for the
+ // others
+ const std::vector<int>& child_ids =
+ static_cast<const UnionType*>(this->type)->child_ids();
Review Comment:
```suggestion
const std::vector<int>& child_ids =
checked_cast<const UnionType*>(this->type)->child_ids();
```
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1429,19 +1477,37 @@ class TestUnionScalar : public ::testing::Test {
void TestValidateErrors() {
// Type code doesn't exist
- AssertValidationFails(ScalarType(alpha_, 0, type_));
- AssertValidationFails(ScalarType(alpha_, 0, type_));
- AssertValidationFails(ScalarType(0, type_));
- AssertValidationFails(ScalarType(alpha_, -42, type_));
- AssertValidationFails(ScalarType(-42, type_));
+ auto scalar = ScalarFromValue(0, alpha_);
+ UnionScalar* union_scalar = static_cast<UnionScalar*>(scalar.get());
+
+ // Invalid type code
Review Comment:
Is this redundant with the previous comment above?
##########
cpp/gdb_arrow.py:
##########
@@ -1463,7 +1464,24 @@ def to_string(self):
return f"{self._format_type()}"
-class UnionScalarPrinter(ScalarPrinter):
+class SparseUnionScalarPrinter(ScalarPrinter):
+ """
+ Pretty-printer for arrow::UnionScalar and subclasses.
+ """
+
+ def to_string(self):
+ type_code = self.val['type_code'].cast(gdb.lookup_type('int'))
+ if not self.is_valid:
+ 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'))
+ return (f"{self._format_type()} of type code {type_code}, "
+ f"value {deref(eval_values[child_id])}")
+
+
+
+class DenseUnionScalarPrinter(ScalarPrinter):
"""
Pretty-printer for arrow::UnionScalar and subclasses.
Review Comment:
```suggestion
Pretty-printer for arrow::DenseUnionScalar and subclasses.
```
##########
cpp/src/arrow/compute/exec/expression.h:
##########
@@ -55,7 +55,7 @@ class ARROW_EXPORT Expression {
std::shared_ptr<Function> function;
const Kernel* kernel = NULLPTR;
std::shared_ptr<KernelState> kernel_state;
- ValueDescr descr;
+ TypeHolder type;
Review Comment:
@bkietz @westonpace Does this look generally ok?
##########
cpp/src/arrow/compute/kernel.cc:
##########
@@ -348,22 +335,30 @@ bool InputType::Equals(const InputType& other) const {
}
}
-bool InputType::Matches(const ValueDescr& descr) const {
- if (shape_ != ValueDescr::ANY && descr.shape != shape_) {
- return false;
- }
+bool InputType::Matches(const TypeHolder& type) const {
Review Comment:
Similarly, taking `const DataType&` would be slightly more efficient still.
##########
cpp/src/arrow/compute/kernels/scalar_cast_test.cc:
##########
@@ -2031,7 +2031,10 @@ TEST(Cast, BinaryToString) {
// N.B. null buffer is not always the same if input sliced
AssertBufferSame(*invalid_utf8, *strings, 0);
- ASSERT_EQ(invalid_utf8->data()->buffers[1].get(), strings->data()->buffers[2].get());
+
+ // ARROW-16757: we no longer zero copy, but the contents are equal
Review Comment:
Rather than the JIRA that introduced the change, would be more useful to reference a JIRA about making this zero-copy again?
##########
cpp/src/arrow/compute/cast.cc:
##########
@@ -69,9 +69,9 @@ void EnsureInitCastTable() { std::call_once(cast_table_initialized, InitCastTabl
// Private version of GetCastFunction with better error reporting
// if the input type is known.
Result<std::shared_ptr<CastFunction>> GetCastFunctionInternal(
- const std::shared_ptr<DataType>& to_type, const DataType* from_type = nullptr) {
+ const TypeHolder& to_type, const DataType* from_type = nullptr) {
Review Comment:
Nit, but why not just take `const DataType&`? We're not storing `to_type` anywhere AFAICT.
##########
cpp/gdb_arrow.py:
##########
@@ -1463,7 +1464,24 @@ def to_string(self):
return f"{self._format_type()}"
-class UnionScalarPrinter(ScalarPrinter):
+class SparseUnionScalarPrinter(ScalarPrinter):
+ """
+ Pretty-printer for arrow::UnionScalar and subclasses.
+ """
+
+ def to_string(self):
+ type_code = self.val['type_code'].cast(gdb.lookup_type('int'))
+ if not self.is_valid:
+ 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'))
Review Comment:
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)
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -186,87 +170,106 @@ const Kernel* DispatchExactImpl(const Function* func,
} // namespace detail
Result<const Kernel*> Function::DispatchExact(
- const std::vector<ValueDescr>& values) const {
+ const std::vector<TypeHolder>& values) const {
if (kind_ == Function::META) {
return Status::NotImplemented("Dispatch for a MetaFunction's Kernels");
}
- RETURN_NOT_OK(CheckArity(values));
+ RETURN_NOT_OK(CheckArity(values.size()));
if (auto kernel = detail::DispatchExactImpl(this, values)) {
return kernel;
}
return detail::NoMatchingKernel(this, values);
}
-Result<const Kernel*> Function::DispatchBest(std::vector<ValueDescr>* values) const {
+Result<const Kernel*> Function::DispatchBest(std::vector<TypeHolder>* values) const {
// TODO(ARROW-11508) permit generic conversions here
return DispatchExact(*values);
}
-Result<Datum> Function::Execute(const std::vector<Datum>& args,
- const FunctionOptions* options, ExecContext* ctx) const {
- return ExecuteInternal(args, /*passed_length=*/-1, options, ctx);
+namespace {
+
+/// \brief Check that each Datum is of a "value" type, which means either
+/// SCALAR, ARRAY, or CHUNKED_ARRAY.
Review Comment:
Uh, can we use a less vague terminology than "value"? For example, "one-dimensional" isn't very pretty but at least quite explicit. Perhaps @lidavidm @westonpace have better suggestions, though ("array-compatible"?).
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -328,8 +321,17 @@ bool ExecBatchIterator::Next(ExecBatch* batch) {
// ----------------------------------------------------------------------
// ExecSpanIterator; to eventually replace ExecBatchIterator
-Status ExecSpanIterator::Init(const ExecBatch& batch, ValueDescr::Shape output_shape,
- int64_t max_chunksize) {
+bool CheckIfAllScalar(const ExecBatch& batch) {
Review Comment:
Make this static or put in the anonymous namespace (if not already there)?
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -995,14 +996,24 @@ class ScalarExecutor : public KernelExecutorImpl<ScalarKernel> {
ExecSpanIterator span_iterator_;
};
+Status CheckCanExecuteChunked(const VectorKernel* kernel) {
Review Comment:
static / anonymous?
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -385,13 +387,20 @@ int64_t ExecSpanIterator::GetNextChunkSpan(int64_t iteration_size, ExecSpan* spa
return iteration_size;
}
-bool ExecSpanIterator::Next(ExecSpan* span) {
- if (position_ == length_) {
- // This also protects from degenerate cases like ChunkedArrays
- // without any chunks
- return false;
+void PromoteExecSpanScalars(ExecSpan* span) {
Review Comment:
Make this static / anonymous as well?
##########
cpp/src/arrow/array/data.h:
##########
@@ -266,16 +266,19 @@ struct ARROW_EXPORT ArraySpan {
int64_t offset = 0;
BufferSpan buffers[3];
+ // 16 bytes of scratch space to enable this ArraySpan to be a view onto
+ // scalar values including binary scalars (where we need to create a buffer
+ // that looks like two 32-bit or 64-bit offsets)
+ uint8_t scratch_space[16];
Review Comment:
I'm not sure I understand actually: why do you need a scratch space if the ArraySpan is a view onto an existing scalar?
(also, you probably want to make this 8-byte aligned?)
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +481,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
Review Comment:
This sounds ok. We already had similarly quandaries with union scalars in the past.
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
Review Comment:
Same question for the various `ClearBuffer` calls...
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
Review Comment:
In any case you can probably simply write:
```suggestion
this->buffers[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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1178414737
Oh sorry yes that’s my mistake
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1175632276
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
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176926571
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.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
lidavidm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r914967972
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +481,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
Review Comment:
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.
##########
cpp/src/arrow/type.h:
##########
@@ -191,7 +191,7 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this<DataType>,
// \brief EXPERIMENTAL: Enable retrieving shared_ptr<DataType> from a const
// context. Implementation requires enable_shared_from_this but we may fix
// this in the future
- std::shared_ptr<DataType> Copy() const {
+ std::shared_ptr<DataType> GetSharedPtr() const {
Review Comment:
Should we also remove the "Implementation requires enable_shared_from_this but we may fix this in the future" note?
##########
cpp/src/arrow/compute/function_internal.h:
##########
@@ -430,6 +434,12 @@ static inline enable_if_same_result<T, std::shared_ptr<DataType>> GenericFromSca
return value->type;
}
+template <typename T>
+static inline enable_if_same_result<T, TypeHolder> GenericFromScalar(
+ const std::shared_ptr<Scalar>& value) {
+ return value->type;
+}
+
Review Comment:
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?)
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1370,27 +1373,61 @@ TEST(TestDictionaryScalar, Cast) {
}
}
+const Scalar& GetUnionValue(const Scalar& value) {
Review Comment:
Should this just be a getter of (Base)UnionScalar? Though I suppose then you'd have to cast to use it in the tests here
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
lidavidm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176610823
> > 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?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176597625
> 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?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916206286
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
Review Comment:
ArraySpan could be reused so setting the buffers to be exactly what you think they should be is the safest thing. Removing ClearBuffer
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916205039
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
Review Comment:
fixin
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916116990
##########
cpp/src/arrow/array/builder_nested.h:
##########
@@ -304,10 +304,12 @@ class ARROW_EXPORT MapBuilder : public ArrayBuilder {
if (!validity || bit_util::GetBit(validity, array.offset + row)) {
ARROW_RETURN_NOT_OK(Append());
const int64_t slot_length = offsets[row + 1] - offsets[row];
+ // Add together the inner StructArray offset to the Map/List offset
+ int64_t key_value_offset = array.child_data[0].offset + offsets[row];
ARROW_RETURN_NOT_OK(key_builder_->AppendArraySlice(
- array.child_data[0].child_data[0], offsets[row], slot_length));
+ array.child_data[0].child_data[0], key_value_offset, slot_length));
ARROW_RETURN_NOT_OK(item_builder_->AppendArraySlice(
- array.child_data[0].child_data[1], offsets[row], slot_length));
+ array.child_data[0].child_data[1], key_value_offset, slot_length));
Review Comment:
Yes — the bug got exposed by the scalar input tests when now pass through the ArraySpan path now that the Scalar path has been deleted (e.g. the `MapLookupScalar` implementation — now passes through MapBuilder).
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] pitrou commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
pitrou commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916202446
##########
cpp/src/arrow/array/data.h:
##########
@@ -266,16 +266,19 @@ struct ARROW_EXPORT ArraySpan {
int64_t offset = 0;
BufferSpan buffers[3];
+ // 16 bytes of scratch space to enable this ArraySpan to be a view onto
+ // scalar values including binary scalars (where we need to create a buffer
+ // that looks like two 32-bit or 64-bit offsets)
+ uint8_t scratch_space[16];
Review Comment:
You can also use [`alignas`](https://en.cppreference.com/w/cpp/language/alignas)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916207297
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
+ }
+ } else if (type_id == Type::STRUCT) {
+ const auto& scalar = checked_cast<const StructScalar&>(value);
+ this->child_data.resize(this->type->num_fields());
+ DCHECK_EQ(this->type->num_fields(), static_cast<int>(scalar.value.size()));
+ for (size_t i = 0; i < scalar.value.size(); ++i) {
+ this->child_data[i].FillFromScalar(*scalar.value[i]);
+ }
+ } else if (is_union(type_id)) {
+ // First buffer is kept null since unions have no validity vector
+ this->buffers[0].data = nullptr;
+ this->buffers[0].size = 0;
+
+ this->buffers[1].data = this->scratch_space;
+ this->buffers[1].size = 1;
+ int8_t* type_codes = reinterpret_cast<int8_t*>(this->scratch_space);
+ type_codes[0] = checked_cast<const UnionScalar&>(value).type_code;
+
+ this->child_data.resize(this->type->num_fields());
+ if (type_id == Type::DENSE_UNION) {
+ const auto& scalar = checked_cast<const DenseUnionScalar&>(value);
+ // Has offset; start 4 bytes in so it's aligned to a 32-bit boundaries
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space + sizeof(int32_t), 1,
+ /*buffer_index=*/2);
+ // We can't "see" the other arrays in the union, but we put the "active"
+ // union array in the right place and fill zero-length arrays for the
+ // others
+ const std::vector<int>& child_ids =
+ static_cast<const UnionType*>(this->type)->child_ids();
Review Comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916207096
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
+
+ span->type = type;
+ span->length = 0;
+ int num_buffers = GetNumBuffers(*type);
+ for (int i = 0; i < num_buffers; ++i) {
+ span->buffers[i].data = span->scratch_space;
+ span->buffers[i].size = 0;
+ }
+
+ for (int i = num_buffers; i < 3; ++i) {
+ span->ClearBuffer(i);
+ }
+
+ // Fill children
+ span->child_data.resize(type->num_fields());
+ for (int i = 0; i < type->num_fields(); ++i) {
+ FillZeroLengthArray(type->field(i)->type().get(), &span->child_data[i]);
+ }
+}
+
+} // namespace internal
+
void ArraySpan::FillFromScalar(const Scalar& value) {
- static const uint8_t kValidByte = 0x01;
- static const uint8_t kNullByte = 0x00;
+ static uint8_t kTrueBit = 0x01;
+ static uint8_t kFalseBit = 0x00;
this->type = value.type.get();
this->length = 1;
- // Populate null count and validity bitmap
+ Type::type type_id = value.type->id();
+
+ // Populate null count and validity bitmap (only for non-union types)
this->null_count = value.is_valid ? 0 : 1;
- this->buffers[0].data = const_cast<uint8_t*>(value.is_valid ? &kValidByte : &kNullByte);
- this->buffers[0].size = 1;
+ if (!is_union(type_id)) {
+ this->buffers[0].data = value.is_valid ? &kTrueBit : &kFalseBit;
+ this->buffers[0].size = 1;
+ }
- if (is_primitive(value.type->id())) {
- const auto& scalar =
- internal::checked_cast<const internal::PrimitiveScalarBase&>(value);
+ if (type_id == Type::BOOL) {
+ const auto& scalar = checked_cast<const BooleanScalar&>(value);
+ this->buffers[1].data = scalar.value ? &kTrueBit : &kFalseBit;
+ this->buffers[1].size = 1;
+ } else if (is_primitive(type_id) || is_decimal(type_id) ||
+ type_id == Type::DICTIONARY) {
+ const auto& scalar = checked_cast<const internal::PrimitiveScalarBase&>(value);
const uint8_t* scalar_data = reinterpret_cast<const uint8_t*>(scalar.view().data());
this->buffers[1].data = const_cast<uint8_t*>(scalar_data);
this->buffers[1].size = scalar.type->byte_width();
+ if (type_id == Type::DICTIONARY) {
+ // Populate dictionary data
+ const auto& dict_scalar = checked_cast<const DictionaryScalar&>(value);
+ this->child_data.resize(1);
+ this->child_data[0].SetMembers(*dict_scalar.value.dictionary->data());
+ }
+ } else if (is_base_binary_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = this->scratch_space;
+ const uint8_t* data_buffer = nullptr;
+ int64_t data_size = 0;
+ if (scalar.is_valid) {
+ data_buffer = scalar.value->data();
+ data_size = scalar.value->size();
+ }
+ if (is_binary_like(type_id)) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, data_size);
+ } else {
+ // is_large_binary_like
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, data_size);
+ }
+ this->buffers[2].data = const_cast<uint8_t*>(data_buffer);
+ this->buffers[2].size = data_size;
+ } else if (type_id == Type::FIXED_SIZE_BINARY) {
+ const auto& scalar = checked_cast<const BaseBinaryScalar&>(value);
+ this->buffers[1].data = const_cast<uint8_t*>(scalar.value->data());
+ this->buffers[1].size = scalar.value->size();
+ } else if (is_list_like(type_id)) {
+ const auto& scalar = checked_cast<const BaseListScalar&>(value);
+
+ int64_t value_length = 0;
+ this->child_data.resize(1);
+ if (scalar.value != nullptr) {
+ // When the scalar is null, scalar.value can also be null
+ this->child_data[0].SetMembers(*scalar.value->data());
+ value_length = scalar.value->length();
+ } else {
+ // Even when the value is null, we still must populate the
+ // child_data to yield a valid array. Tedious
+ internal::FillZeroLengthArray(this->type->field(0)->type().get(),
+ &this->child_data[0]);
+ }
+
+ if (type_id == Type::LIST || type_id == Type::MAP) {
+ SetOffsetsForScalar<int32_t>(this, this->scratch_space, value_length);
+ } else if (type_id == Type::LARGE_LIST) {
+ SetOffsetsForScalar<int64_t>(this, this->scratch_space, value_length);
+ } else {
+ // FIXED_SIZE_LIST: does not have a second buffer
+ this->buffers[1].data = nullptr;
+ this->buffers[1].size = 0;
+ }
+ } else if (type_id == Type::STRUCT) {
+ const auto& scalar = checked_cast<const StructScalar&>(value);
+ this->child_data.resize(this->type->num_fields());
+ DCHECK_EQ(this->type->num_fields(), static_cast<int>(scalar.value.size()));
+ for (size_t i = 0; i < scalar.value.size(); ++i) {
+ this->child_data[i].FillFromScalar(*scalar.value[i]);
+ }
+ } else if (is_union(type_id)) {
+ // First buffer is kept null since unions have no validity vector
+ this->buffers[0].data = nullptr;
+ this->buffers[0].size = 0;
+
+ this->buffers[1].data = this->scratch_space;
+ this->buffers[1].size = 1;
+ int8_t* type_codes = reinterpret_cast<int8_t*>(this->scratch_space);
+ type_codes[0] = checked_cast<const UnionScalar&>(value).type_code;
+
+ this->child_data.resize(this->type->num_fields());
+ if (type_id == Type::DENSE_UNION) {
+ const auto& scalar = checked_cast<const DenseUnionScalar&>(value);
+ // Has offset; start 4 bytes in so it's aligned to a 32-bit boundaries
Review Comment:
`scratch_space[0]` was populated above
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916203979
##########
cpp/src/arrow/scalar_test.cc:
##########
@@ -1370,27 +1373,61 @@ TEST(TestDictionaryScalar, Cast) {
}
}
+const Scalar& GetUnionValue(const Scalar& value) {
Review Comment:
The value representation is different -- if this becomes a burden we could add a virtual value accessor to `UnionScalar`
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1175728038
@wesm Sure! I'll do it.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916204529
##########
cpp/src/arrow/array/data.cc:
##########
@@ -174,27 +175,197 @@ void ArraySpan::SetMembers(const ArrayData& data) {
}
}
+template <typename offset_type>
+void SetOffsetsForScalar(ArraySpan* span, uint8_t* buffer, int64_t value_size,
+ int buffer_index = 1) {
+ auto offsets = reinterpret_cast<offset_type*>(buffer);
+ offsets[0] = 0;
+ offsets[1] = static_cast<offset_type>(value_size);
+ span->buffers[buffer_index].data = buffer;
+ span->buffers[buffer_index].size = 2 * sizeof(offset_type);
+}
+
+int GetNumBuffers(const DataType& type) {
+ switch (type.id()) {
+ case Type::NA:
+ case Type::STRUCT:
+ case Type::FIXED_SIZE_LIST:
+ return 1;
+ case Type::BINARY:
+ case Type::LARGE_BINARY:
+ case Type::STRING:
+ case Type::LARGE_STRING:
+ case Type::DENSE_UNION:
+ return 3;
+ case Type::EXTENSION:
+ // The number of buffers depends on the storage type
+ return GetNumBuffers(
+ *internal::checked_cast<const ExtensionType&>(type).storage_type());
+ default:
+ // Everything else has 2 buffers
+ return 2;
+ }
+}
+
+namespace internal {
+
+void FillZeroLengthArray(const DataType* type, ArraySpan* span) {
+ memset(span->scratch_space, 0x00, 16);
Review Comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r916212441
##########
cpp/src/arrow/compute/cast.cc:
##########
@@ -213,36 +201,48 @@ Result<const Kernel*> CastFunction::DispatchExact(
return candidate_kernels[0];
}
+Result<std::shared_ptr<CastFunction>> GetCastFunction(const TypeHolder& to_type) {
+ return internal::GetCastFunctionInternal(to_type);
Review Comment:
done
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -186,87 +170,106 @@ const Kernel* DispatchExactImpl(const Function* func,
} // namespace detail
Result<const Kernel*> Function::DispatchExact(
- const std::vector<ValueDescr>& values) const {
+ const std::vector<TypeHolder>& values) const {
if (kind_ == Function::META) {
return Status::NotImplemented("Dispatch for a MetaFunction's Kernels");
}
- RETURN_NOT_OK(CheckArity(values));
+ RETURN_NOT_OK(CheckArity(values.size()));
if (auto kernel = detail::DispatchExactImpl(this, values)) {
return kernel;
}
return detail::NoMatchingKernel(this, values);
}
-Result<const Kernel*> Function::DispatchBest(std::vector<ValueDescr>* values) const {
+Result<const Kernel*> Function::DispatchBest(std::vector<TypeHolder>* values) const {
// TODO(ARROW-11508) permit generic conversions here
return DispatchExact(*values);
}
-Result<Datum> Function::Execute(const std::vector<Datum>& args,
- const FunctionOptions* options, ExecContext* ctx) const {
- return ExecuteInternal(args, /*passed_length=*/-1, options, ctx);
+namespace {
+
+/// \brief Check that each Datum is of a "value" type, which means either
+/// SCALAR, ARRAY, or CHUNKED_ARRAY.
+Status CheckAllValues(const std::vector<Datum>& values) {
+ for (const auto& value : values) {
+ if (!value.is_value()) {
+ return Status::Invalid("Tried executing function with non-value type: ",
+ value.ToString());
+ }
+ }
+ return Status::OK();
}
-Result<Datum> Function::Execute(const ExecBatch& batch, const FunctionOptions* options,
- ExecContext* ctx) const {
- return ExecuteInternal(batch.values, batch.length, options, ctx);
+Status CheckOptions(const Function& function, const FunctionOptions* options) {
+ if (options == nullptr && function.doc().options_required) {
+ return Status::Invalid("Function '", function.name(),
+ "' cannot be called without options");
+ }
+ return Status::OK();
}
-Result<Datum> Function::ExecuteInternal(const std::vector<Datum>& args,
- int64_t passed_length,
- const FunctionOptions* options,
- ExecContext* ctx) const {
+Result<Datum> ExecuteInternal(const Function& func, std::vector<Datum> args,
Review Comment:
We should investigate later, especially for the overly common cases where execution has 1, 2, or 3 arguments.
##########
cpp/src/arrow/compute/cast.cc:
##########
@@ -69,9 +69,9 @@ void EnsureInitCastTable() { std::call_once(cast_table_initialized, InitCastTabl
// Private version of GetCastFunction with better error reporting
// if the input type is known.
Result<std::shared_ptr<CastFunction>> GetCastFunctionInternal(
- const std::shared_ptr<DataType>& to_type, const DataType* from_type = nullptr) {
+ const TypeHolder& to_type, const DataType* from_type = nullptr) {
Review Comment:
done
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -995,14 +996,24 @@ class ScalarExecutor : public KernelExecutorImpl<ScalarKernel> {
ExecSpanIterator span_iterator_;
};
+Status CheckCanExecuteChunked(const VectorKernel* kernel) {
Review Comment:
done
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +480,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
+
+ // The value index corresponding to the active type code
+ int child_id;
+
+ SparseUnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type);
+
+ /// \brief Construct a SparseUnionScalar from a single value, versus having
+ /// to construct a vector of scalars
+ static std::shared_ptr<Scalar> FromValue(std::shared_ptr<Scalar> value, int field_index,
+ std::shared_ptr<DataType> type);
};
struct ARROW_EXPORT DenseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = DenseUnionType;
+
+ // For DenseUnionScalar, we can make a valid ArraySpan of length 1 from this
+ // scalar
+ using ValueType = std::shared_ptr<Scalar>;
+ ValueType value;
Review Comment:
done
##########
cpp/src/arrow/scalar.h:
##########
@@ -479,37 +480,52 @@ struct ARROW_EXPORT StructScalar : public Scalar {
Result<std::shared_ptr<Scalar>> field(FieldRef ref) const;
- StructScalar(ValueType value, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)) {}
+ StructScalar(ValueType value, std::shared_ptr<DataType> type, bool is_valid = true)
+ : Scalar(std::move(type), is_valid), value(std::move(value)) {}
static Result<std::shared_ptr<StructScalar>> Make(ValueType value,
std::vector<std::string> field_names);
-
- explicit StructScalar(std::shared_ptr<DataType> type) : Scalar(std::move(type)) {}
};
struct ARROW_EXPORT UnionScalar : public Scalar {
- using Scalar::Scalar;
- using ValueType = std::shared_ptr<Scalar>;
-
- ValueType value;
int8_t type_code;
- UnionScalar(int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), false), type_code(type_code) {}
-
- UnionScalar(ValueType value, int8_t type_code, std::shared_ptr<DataType> type)
- : Scalar(std::move(type), true), value(std::move(value)), type_code(type_code) {}
+ protected:
+ UnionScalar(std::shared_ptr<DataType> type, int8_t type_code, bool is_valid)
+ : Scalar(std::move(type), is_valid), type_code(type_code) {}
};
struct ARROW_EXPORT SparseUnionScalar : public UnionScalar {
- using UnionScalar::UnionScalar;
using TypeClass = SparseUnionType;
+
+ // Even though only one of the union values is relevant for this scalar, we
+ // nonetheless construct a vector of scalars, one per union value, to have
+ // enough data to reconstruct a valid ArraySpan of length 1 from this scalar
+ using ValueType = std::vector<std::shared_ptr<Scalar>>;
+ ValueType value;
+
+ // The value index corresponding to the active type code
+ int child_id;
Review Comment:
yes, and adding it as virtual on UnionScalar
##########
cpp/src/arrow/compute/kernel.h:
##########
@@ -227,21 +208,16 @@ class ARROW_EXPORT InputType {
/// \brief Render a human-readable string representation.
std::string ToString() const;
- /// \brief Return true if the value matches this argument kind in type
- /// and shape.
+ /// \brief Return true if the Datum matches this argument kind in
+ /// type (and only allows scalar or array-like Datums).
bool Matches(const Datum& value) const;
- /// \brief Return true if the value descriptor matches this argument kind in
- /// type and shape.
- bool Matches(const ValueDescr& value) const;
+ /// \brief Return true if the type matches this InputType
+ bool Matches(const TypeHolder& type) const;
Review Comment:
yes definitely, will fix
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -328,8 +321,17 @@ bool ExecBatchIterator::Next(ExecBatch* batch) {
// ----------------------------------------------------------------------
// ExecSpanIterator; to eventually replace ExecBatchIterator
-Status ExecSpanIterator::Init(const ExecBatch& batch, ValueDescr::Shape output_shape,
- int64_t max_chunksize) {
+bool CheckIfAllScalar(const ExecBatch& batch) {
Review Comment:
done
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -784,30 +811,28 @@ class ScalarExecutor : public KernelExecutorImpl<ScalarKernel> {
Datum WrapResults(const std::vector<Datum>& inputs,
const std::vector<Datum>& outputs) override {
- if (output_descr_.shape == ValueDescr::SCALAR) {
- // TODO(wesm): to remove, see ARROW-16757
- DCHECK_EQ(outputs.size(), 1);
- // Return as SCALAR
- return outputs[0];
+ // If execution yielded multiple chunks (because large arrays were split
+ // based on the ExecContext parameters, then the result is a ChunkedArray
+ if (HaveChunkedArray(inputs) || outputs.size() > 1) {
+ return ToChunkedArray(outputs, output_type_);
} else {
- // If execution yielded multiple chunks (because large arrays were split
- // based on the ExecContext parameters, then the result is a ChunkedArray
- if (HaveChunkedArray(inputs) || outputs.size() > 1) {
- return ToChunkedArray(outputs, output_descr_.type);
- } else if (outputs.size() == 1) {
- // Outputs have just one element
- return outputs[0];
- } else {
- // XXX: In the case where no outputs are omitted, is returning a 0-length
- // array always the correct move?
- return MakeArrayOfNull(output_descr_.type, /*length=*/0,
- exec_context()->memory_pool())
- .ValueOrDie();
- }
+ // Outputs have just one element
+ return outputs[0];
}
}
protected:
+ Status EmitResult(std::shared_ptr<ArrayData> out, ExecListener* listener) {
Review Comment:
It's move-d elsewhere, though I'm guessing that an `std::move` operation is never going to show up in any profile output
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -186,87 +170,106 @@ const Kernel* DispatchExactImpl(const Function* func,
} // namespace detail
Result<const Kernel*> Function::DispatchExact(
- const std::vector<ValueDescr>& values) const {
+ const std::vector<TypeHolder>& values) const {
if (kind_ == Function::META) {
return Status::NotImplemented("Dispatch for a MetaFunction's Kernels");
}
- RETURN_NOT_OK(CheckArity(values));
+ RETURN_NOT_OK(CheckArity(values.size()));
if (auto kernel = detail::DispatchExactImpl(this, values)) {
return kernel;
}
return detail::NoMatchingKernel(this, values);
}
-Result<const Kernel*> Function::DispatchBest(std::vector<ValueDescr>* values) const {
+Result<const Kernel*> Function::DispatchBest(std::vector<TypeHolder>* values) const {
// TODO(ARROW-11508) permit generic conversions here
return DispatchExact(*values);
}
-Result<Datum> Function::Execute(const std::vector<Datum>& args,
- const FunctionOptions* options, ExecContext* ctx) const {
- return ExecuteInternal(args, /*passed_length=*/-1, options, ctx);
+namespace {
+
+/// \brief Check that each Datum is of a "value" type, which means either
+/// SCALAR, ARRAY, or CHUNKED_ARRAY.
Review Comment:
This comment predates this PR. I'll change it though
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -275,9 +278,9 @@ Result<Datum> Function::ExecuteInternal(const std::vector<Datum>& args,
bool all_same_length = false;
int64_t inferred_length = detail::InferBatchLength(input.values, &all_same_length);
input.length = inferred_length;
- if (kind() == Function::SCALAR) {
+ if (func.kind() == Function::SCALAR) {
DCHECK(passed_length == -1 || passed_length == inferred_length);
Review Comment:
done
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176947212
> I'll try clearing GitHub Actions cache for MinGW 32/64 C++ builds.
I couldn't do this...
```console
$ 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.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] kou commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
kou commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1178403030
It seems that my change was lost by force-push. I'll add it again.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1178179237
Thanks everyone. I'll merge this once the CI passes so I can get busy with the next items
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on PR #13521:
URL: https://github.com/apache/arrow/pull/13521#issuecomment-1176596152
> 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.)
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] wesm commented on a diff in pull request #13521: ARROW-16757: [C++] Remove "scalar" output modality for ScalarKernel implementations, remove ValueDescr class
Posted by GitBox <gi...@apache.org>.
wesm commented on code in PR #13521:
URL: https://github.com/apache/arrow/pull/13521#discussion_r915183045
##########
cpp/src/arrow/type.h:
##########
@@ -191,7 +191,7 @@ class ARROW_EXPORT DataType : public std::enable_shared_from_this<DataType>,
// \brief EXPERIMENTAL: Enable retrieving shared_ptr<DataType> from a const
// context. Implementation requires enable_shared_from_this but we may fix
// this in the future
- std::shared_ptr<DataType> Copy() const {
+ std::shared_ptr<DataType> GetSharedPtr() const {
Review Comment:
will fix
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org