[GitHub] [arrow] wesm commented on a change in pull request #7635: ARROW-1587: [C++] implement fill null
wesm commented on a change in pull request #7635:
URL: https://github.com/apache/arrow/pull/7635#discussion_r449802099
##
File path: cpp/src/arrow/compute/api_scalar.h
##
@@ -259,6 +259,27 @@ Result IsValid(const Datum& values, ExecContext*
ctx = NULLPTR);
ARROW_EXPORT
Result IsNull(const Datum& values, ExecContext* ctx = NULLPTR);
+struct ARROW_EXPORT FillNullOptions : public FunctionOptions {
+ explicit FillNullOptions(Datum fill_value) :
fill_value(std::move(fill_value)) {}
+
+ Datum fill_value;
+};
Review comment:
See comments above. I think this should be implemented as a binary
function since the "fill values" could be provided by an array. This also will
allow the execution layer to (in the near future) insert implicit casts where
needed
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [arrow] wesm commented on a change in pull request #7635: ARROW-1587: [C++] implement fill null
wesm commented on a change in pull request #7635:
URL: https://github.com/apache/arrow/pull/7635#discussion_r449877611
##
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##
@@ -0,0 +1,223 @@
+
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/array/array_base.h"
+#include "arrow/array/builder_primitive.h"
+#include "arrow/compute/api_scalar.h"
+#include "arrow/compute/kernels/common.h"
+#include "arrow/util/bit_util.h"
+#include "arrow/util/bitmap_writer.h"
+#include "arrow/visitor_inline.h"
+
+namespace arrow {
+
+namespace compute {
+namespace internal {
+namespace {
+
+template
+using enable_if_supports_fill_null = enable_if_t::value, R>;
+
+template
+struct FillNullState : public KernelState {
+ explicit FillNullState(MemoryPool* pool) {}
+
+ Status Init(const FillNullOptions& options) {
+fill_value = options.fill_value.scalar();
+return Status::OK();
+ }
+
+ std::shared_ptr fill_value;
+};
+
+template <>
+struct FillNullState : public KernelState {
+ explicit FillNullState(MemoryPool*) {}
+
+ Status Init(const FillNullOptions& options) { return Status::OK(); }
+
+ std::shared_ptr fill_value;
+};
+
+struct InitFillNullStateVisitor {
+ KernelContext* ctx;
+ const FillNullOptions* options;
+ std::unique_ptr result;
+
+ InitFillNullStateVisitor(KernelContext* ctx, const FillNullOptions* options)
+ : ctx(ctx), options(options) {}
+
+ template
+ Status Init() {
+using StateType = FillNullState;
+result.reset(new StateType(ctx->exec_context()->memory_pool()));
+return static_cast(result.get())->Init(*options);
+ }
+
+ Status Visit(const DataType&) { return Init(); }
+
+ template
+ enable_if_supports_fill_null Visit(const Type&) {
+return Init();
+ }
+
+ Status GetResult(std::unique_ptr* out) {
+RETURN_NOT_OK(VisitTypeInline(*options->fill_value.type(), this));
+*out = std::move(result);
+return Status::OK();
+ }
+};
+
+std::unique_ptr InitFillNull(KernelContext* ctx,
+ const KernelInitArgs& args) {
+ InitFillNullStateVisitor visitor{ctx,
+ static_cast(args.options)};
+ std::unique_ptr result;
+ ctx->SetStatus(visitor.GetResult());
+ return result;
+}
+
+struct ScalarFillVisitor {
+ KernelContext* ctx;
+ const ArrayData& data;
+ Datum* out;
+
+ ScalarFillVisitor(KernelContext* ctx, const ArrayData& data, Datum* out)
+ : ctx(ctx), data(data), out(out) {}
+
+ Status Visit(const DataType&) {
+ArrayData* out_arr = out->mutable_array();
+*out_arr = data;
+return Status::OK();
+ }
+
+ Status Visit(const BooleanType&) {
+const auto& state = checked_cast&>(*ctx->state());
+bool value = UnboxScalar::Unbox(*state.fill_value);
+ArrayData* out_arr = out->mutable_array();
+FirstTimeBitmapWriter bit_writer(out_arr->buffers[1]->mutable_data(),
out_arr->offset,
+ out_arr->length);
+FirstTimeBitmapWriter
bit_writer_validity(out_arr->buffers[0]->mutable_data(),
+ out_arr->offset,
out_arr->length);
+if (data.null_count != 0) {
+ BitmapReader bit_reader(data.buffers[1]->data(), data.offset,
data.length);
+ BitmapReader bit_reader_validity(data.buffers[0]->data(), data.offset,
data.length);
+ for (int64_t i = 0; i < data.length; i++) {
+if (bit_reader_validity.IsNotSet()) {
+ if (value == true) {
+bit_writer.Set();
+ } else {
+bit_writer.Clear();
+ }
+ bit_writer_validity.Set();
+} else {
+ if (bit_reader.IsSet()) {
+bit_writer.Set();
+ } else {
+bit_writer.Clear();
+ }
+ bit_writer_validity.Set();
+}
+bit_reader.Next();
+bit_writer.Next();
+bit_reader_validity.Next();
+bit_writer_validity.Next();
+ }
+ bit_writer_validity.Finish();
+ bit_writer.Finish();
+} else {
+ *out_arr = data;
+}
+return Status::OK();
+ }
+
+ template
+ enable_if_supports_fill_null Visit(const Type&) {
+using T = typename GetViewType::T;
+const
[GitHub] [arrow] wesm commented on a change in pull request #7635: ARROW-1587: [C++] implement fill null
wesm commented on a change in pull request #7635:
URL: https://github.com/apache/arrow/pull/7635#discussion_r449802049
##
File path: cpp/src/arrow/compute/api_scalar.cc
##
@@ -126,5 +126,24 @@ Result Compare(const Datum& left, const Datum&
right, CompareOptions opti
SCALAR_EAGER_UNARY(IsValid, "is_valid")
SCALAR_EAGER_UNARY(IsNull, "is_null")
+Result FillNull(const Datum& values, const Datum& fill_value,
ExecContext* ctx) {
+ if (!values.is_arraylike()) {
+return Status::Invalid("Values must be Array or ChunkedArray");
+ }
+
+ if (!fill_value.is_scalar()) {
+return Status::Invalid("fill value must be a scalar");
+ }
+
+ if (!values.type()->Equals(fill_value.type())) {
+std::stringstream ss;
+ss << "Array type didn't match type of fill value: " <<
values.type()->ToString()
+ << " vs " << fill_value.type()->ToString();
+return Status::Invalid(ss.str());
+ }
Review comment:
None of these input validation checks should be here. Instead, the
kernel should be implemented as an `Arity::Binary()` kernel with input
validation handled by the kernel dispatch / executor layer. It's fine if the
initial version has the type signature `Array/Scalar` instead of `Any/Any`
##
File path: cpp/src/arrow/compute/kernels/scalar_fill_null.cc
##
@@ -0,0 +1,223 @@
+
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#include "arrow/array/array_base.h"
+#include "arrow/array/builder_primitive.h"
+#include "arrow/compute/api_scalar.h"
+#include "arrow/compute/kernels/common.h"
+#include "arrow/util/bit_util.h"
+#include "arrow/util/bitmap_writer.h"
+#include "arrow/visitor_inline.h"
+
+namespace arrow {
+
+namespace compute {
+namespace internal {
+namespace {
+
+template
+using enable_if_supports_fill_null = enable_if_t::value, R>;
+
+template
+struct FillNullState : public KernelState {
+ explicit FillNullState(MemoryPool* pool) {}
+
+ Status Init(const FillNullOptions& options) {
+fill_value = options.fill_value.scalar();
+return Status::OK();
+ }
+
+ std::shared_ptr fill_value;
+};
+
+template <>
+struct FillNullState : public KernelState {
+ explicit FillNullState(MemoryPool*) {}
+
+ Status Init(const FillNullOptions& options) { return Status::OK(); }
+
+ std::shared_ptr fill_value;
+};
+
+struct InitFillNullStateVisitor {
+ KernelContext* ctx;
+ const FillNullOptions* options;
+ std::unique_ptr result;
+
+ InitFillNullStateVisitor(KernelContext* ctx, const FillNullOptions* options)
+ : ctx(ctx), options(options) {}
+
+ template
+ Status Init() {
+using StateType = FillNullState;
+result.reset(new StateType(ctx->exec_context()->memory_pool()));
+return static_cast(result.get())->Init(*options);
+ }
+
+ Status Visit(const DataType&) { return Init(); }
+
+ template
+ enable_if_supports_fill_null Visit(const Type&) {
+return Init();
+ }
+
+ Status GetResult(std::unique_ptr* out) {
+RETURN_NOT_OK(VisitTypeInline(*options->fill_value.type(), this));
+*out = std::move(result);
+return Status::OK();
+ }
+};
+
+std::unique_ptr InitFillNull(KernelContext* ctx,
+ const KernelInitArgs& args) {
+ InitFillNullStateVisitor visitor{ctx,
+ static_cast(args.options)};
+ std::unique_ptr result;
+ ctx->SetStatus(visitor.GetResult());
+ return result;
+}
+
+struct ScalarFillVisitor {
+ KernelContext* ctx;
+ const ArrayData& data;
+ Datum* out;
+
+ ScalarFillVisitor(KernelContext* ctx, const ArrayData& data, Datum* out)
+ : ctx(ctx), data(data), out(out) {}
+
+ Status Visit(const DataType&) {
+ArrayData* out_arr = out->mutable_array();
+*out_arr = data;
+return Status::OK();
+ }
+
+ Status Visit(const BooleanType&) {
+const auto& state = checked_cast&>(*ctx->state());
+bool value = UnboxScalar::Unbox(*state.fill_value);
+ArrayData* out_arr = out->mutable_array();
+FirstTimeBitmapWriter bit_writer(out_arr->buffers[1]->mutable_data(),
out_arr->offset,
+ out_arr->length);
+FirstTimeBitmapWriter
bit_writer_validity(out_arr->buffers[0]->mutable_data(),
+
