westonpace commented on a change in pull request #9323: URL: https://github.com/apache/arrow/pull/9323#discussion_r568264655
########## File path: cpp/src/arrow/pretty_print.cc ########## @@ -670,4 +670,49 @@ Status PrettyPrint(const Schema& schema, const PrettyPrintOptions& options, return Status::OK(); } +void GdbPrintArray(const Array& arr, int indent) { Review comment: Removed. ########## File path: cpp/src/arrow/compute/api_vector.h ########## @@ -63,6 +63,25 @@ enum class SortOrder { Descending, }; +struct DictionaryEncodeOptions : public FunctionOptions { + /// Configure how null values will be encoded + enum NullEncodingBehavior { + /// the null value will be added to the dictionary with a proper index + ENCODE, + /// the null value will be masked in the indices array + MASK, + /// the null value will not be included in the dictionary + SKIP Review comment: I removed SKIP and added some comments similar to those in filter. ########## File path: cpp/src/arrow/compute/api_vector.h ########## @@ -296,7 +315,8 @@ Result<std::shared_ptr<StructArray>> ValueCounts(const Datum& value, /// \since 1.0.0 /// \note API not yet finalized ARROW_EXPORT -Result<Datum> DictionaryEncode(const Datum& data, ExecContext* ctx = NULLPTR); +Result<Datum> DictionaryEncode(const Datum& data, const DictionaryEncodeOptions& options, Review comment: Done, and cleaned up all the other call sites explicitly stating the default options. ########## File path: cpp/src/arrow/compute/kernels/vector_hash.cc ########## @@ -451,8 +487,12 @@ struct HashKernelTraits<Type, Action, enable_if_has_string_view<Type>> { template <typename Type, typename Action> std::unique_ptr<HashKernel> HashInitImpl(KernelContext* ctx, const KernelInitArgs& args) { using HashKernelType = typename HashKernelTraits<Type, Action>::HashKernel; - auto result = ::arrow::internal::make_unique<HashKernelType>(args.inputs[0].type, - ctx->memory_pool()); + DictionaryEncodeOptions options; + if (auto options_ptr = static_cast<const DictionaryEncodeOptions*>(args.options)) { Review comment: I moved the options decoding into Action although the `HashKernel` still needed to be aware of it (to decide if `null` should go in the dictionary or not) so it changed the kernel->action interface slightly (added `ShouldEncodeNulls`) ########## File path: cpp/src/arrow/dataset/partition.cc ########## @@ -625,9 +621,17 @@ class StructDictionary { private: Status AddOne(Datum column, std::shared_ptr<Int32Array>* fused_indices) { - if (column.type()->id() != Type::DICTIONARY) { - ARROW_ASSIGN_OR_RAISE(column, compute::DictionaryEncode(std::move(column))); + if (column.type()->id() == Type::DICTIONARY) { + // compute::DictionaryEncode doesn't support dictionary and, even if it did, it + // would be a null op and return a flat dictionary. In order to group by dictionary + // we would need to be able to create a nested dictionary. + return Status::NotImplemented( + "Cannot use column of type dictionary as grouping criteria"); Review comment: Yes, I keep thinking of dictionary arrays as arrays of dictionaries [{"a": 1, "b": 2}, ...] and not simply a different encoding. My brain was off on a tangent when I wrote this column. I added the logic to encode nulls by decoding (casting) the dictionary first and then re-encoding. I also added a test case for this. ---------------------------------------------------------------- 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