GH-44866: [C++] Minor enhance the doc and impl of DictionaryArray::Transpose #44867
Conversation
|
|
cpp/src/arrow/array/array_dict.h
Outdated
| @@ -90,7 +90,9 @@ class ARROW_EXPORT DictionaryArray : public Array { | |||
| /// \param[in] type the new type object | |||
| /// \param[in] dictionary the new dictionary | |||
| /// \param[in] transpose_map transposition array of this array's indices | |||
| /// into the target array's indices | |||
| /// into the target array's indices. The value of transpose_map should | |||
| /// be in the range [0, this->length()). | |||
There was a problem hiding this comment.
I don't know whether this should be DCHECK-ed
github-actions
bot
added
awaiting committer review
github-actions
bot
added
awaiting merge
|
@pitrou Would you mind take a look? |
| @@ -320,7 +320,7 @@ Result<std::shared_ptr<Array>> DictionaryArray::Transpose( | |||
| ARROW_ASSIGN_OR_RAISE(auto transposed, | |||
| TransposeDictIndices(data_, data_->type, type, dictionary->data(), | |||
| transpose_map, pool)); | |||
| return MakeArray(std::move(transposed)); | |||
| return MakeArray(transposed); | |||
There was a problem hiding this comment.
because MakeArray takes a const ref as argument..
There was a problem hiding this comment.
Well, perhaps MakeArray will be fixed at some point, but there's no reason to remove the move here? It's not worse in any case?
cpp/src/arrow/array/array_dict.h
Outdated
| /// into the target array's indices. The value of transpose_map should | ||
| /// be in the range [0, this->length()). And the dictionary array's | ||
| /// indices in the target array's indices should be in the range | ||
| /// of [0, dictionary->length()). |
There was a problem hiding this comment.
Those two sentences are difficult to understand, and I'm not sure they are correct either. Are you sure you want to add this?
There was a problem hiding this comment.
It's from the user-case here #44827 . I think we should try describe this well
There was a problem hiding this comment.
I'm against this: 1) this should be obvious from the function description 2) your description seems wrong (the transpose_map values should be in [0, dictionary->length()), not [0, this->length())).
There was a problem hiding this comment.
You're right, one is dictionary indice, another is dictionary's dictionary's indices...
Rationale for this change
Minor enhance the doc and impl of DictionaryArray::Transpose
What changes are included in this PR?
Minor enhance the doc and impl of DictionaryArray::Transpose
Are these changes tested?
Covered by existing
Are there any user-facing changes?
No