Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2162036557 > Imagine converting an instance of `list` to string. You need a tree of string formatters prepared before the loop and it has to follow the same shape of the `DataType`. Doing it value-by-value is just as hard, but slower. I think I understand what you mean. Thank you for your response! If you proceed with the work on the `StringFormatter` as [mentioned](https://github.com/apache/arrow/pull/41894#pullrequestreview-2111760063), I will review it and continue with the process. Thank you once 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2161918784 > For example, the Scalar::ToString function used CastTo(utf8()) to handle ToString(), and I thought I could solve this by implementing the Cast Kernel for String Type. This is why I attempted to create these kernels. OK, it's good that there is precedent in the codebase for the string conversions to be this way. > Thank you again for the in-depth review. I feel a bit overwhelmed about how to proceed. I'm sorry. Anything that touches all the Arrow types becomes very overwhelming. And when you're dealing with nested types you move away from the world where static dispatching works and you need a way to perform dynamic dispatching (either via `switch` on type ids or virtual calls — vtables). Imagine converting an instance of `list` to string. You need a tree of string formatters prepared before the loop and it has to follow the same shape of the `DataType`. Doing it value-by-value is just as hard, but slower. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on PR #41827:
URL: https://github.com/apache/arrow/pull/41827#issuecomment-2145252898
@felipecrv Thank you for your review.
The main reason for handling this issue is to address the following problem:
- #35560
I have implemented `Cast` Kernels for the necessary types. And I wanted to
tackle the next issue to finally remove the `Scalar::CastTo` function:
- #39182
However, I encountered a problem (such as in
https://github.com/apache/arrow/pull/39192#discussion_r1477056138) where
various languages binding C++ produced errors (e.g., Python, Ruby). Recently, I
found a hint that implementing the `${TYPES}` to String Cast Kernel could solve
the issue while trying to fix the recent problems again.
As you mentioned, the long-term aspect of `"something for humans to read?
JSON-like notation?"` is important, but the primary problem I wanted to solve
was to remove the `Scalar::CastTo` function while maintaining compatibility
with the existing code.
For example, the `Scalar::ToString` function used `CastTo(utf8())` to handle
`ToString()`, and I thought I could solve this by implementing the `Cast`
Kernel for String Type. This is why I attempted to create these kernels.
https://github.com/apache/arrow/blob/54bece3d4cf28df3e7bd92ed27f62d705a5cac96/cpp/src/arrow/scalar.cc#L978-L996
Thank you again for the in-depth review. I feel a bit overwhelmed about how
to proceed.
Solving new issues is important, but I also want to finish the PRs I have
already submitted. It feels like I'm not making much progress. Anyway, I will
think more about it based on your review.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2144018035 @llama90 what is the motivation for working on these kernels? It's hard to define what would be a desired string representation of nested types (something for humans to read? JSON-like notation?) and it's quite a lot of work. If you're looking for compute kernel work, may I interest you in making sure run-end encoded [1] types are handled everywhere? Scalar kernels can be fixed generically by applying the transformation to the values and keeping the same run-ends. There might be something like this for handling DICTIONARY automatically as well that you can use for inspiration. [1] https://arrow.apache.org/docs/format/Columnar.html#run-end-encoded-layout -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1623650513
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
A string formatter for unions should allocate a vector of string formatters
that can do virtual dispatching (and deal with nesting themselves as well).
`StringFormatter` performs static dispatch which allows loop-specialization
for the non-nested types. But for nested types we will need to setup a vector
of `VirtualStringFormatter` (which is actually a tree) so that all the
"switching on the type" happens at construction time (beginning of the loop)
and invocations inside the loop are following the same function pointers from
the vtables.
```cpp
// in header
class VirtualStringFormatter {
virtual ... = 0;
};
Result> MakeFormatter(
const std::shared_ptr& type);
// in an anon namespace of the .cpp
// one sub-class per `Type::type`
class StringFormatter : public VirtualStringFormatter {
}
// you can use templates to cover most cases delegating to StringFormatter
class UnionStringFormatter : ...
```
This hierarchy would be similar to the builder class hierarchy.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1623498992
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
> In this way, we can unify it with other type's implementations and shield
the logic of converting strings in the current file.
True, but that can prevent optimizations in the future. The approach of
taking a scalar function and turning it into an array function by mapping
—`array::map(scalar_function: scalar -> scalar) -> array` — is appealing but
prevents vectorization techniques.
UPDATE: that's what we will do here because the set of unions and their
parametrizations is infinite, but `StringFormatter` is not
the way to go because it would have to `switch` on the type for every
invocation of the formatter.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1623506157
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
+ if (input.IsNull(i)) {
+RETURN_NOT_OK(builder.AppendNull());
+continue;
+ }
+
+ const int8_t type_id = type_ids[i];
+ const auto& field = union_type.field(union_type.child_ids()[type_id]);
+ const ArraySpan& child_span =
input.child_data[union_type.child_ids()[type_id]];
+
+ std::shared_ptr child_scalar;
+ auto child_index = union_type.mode() == UnionMode::DENSE ? offsets[i] :
i;
Review Comment:
This should be a template parameter (a `UnionMode::type`) so we can
specialize with `if constexpr` checks in the loop.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
felipecrv commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1623498992
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
> In this way, we can unify it with other type's implementations and shield
the logic of converting strings in the current file.
True, but that can prevent optimizations in the future. The approach of
taking a scalar function and turning it into an array function by mapping
—`array::map(scalar_function: scalar -> scalar) -> array` — is appealing but
prevents vectorization techniques.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
ZhangHuiGui commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2136819346 I'm ok to move these issues forward with this way. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2135226741 I have created an issue to add support for nested types in StringFormatter. It seems like addressing this first would be beneficial. - #41864 -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2133502365 I have been examining the code with the intention of using `StringFormatter`, as it seems like a reasonable choice. However, currently, `StringFormatter` mostly handles primitive types and takes time to support complex types such as `Union`, `Struct`, and `List-Like` types. Not only did I review the code, but I also attempted to modify it. These modifications are related to the Legacy code such as `CastImpl`. For reference, please see: https://github.com/apache/arrow/blob/ff9921ffa89585be69ae85674bb365d03cb22ba4/cpp/src/arrow/scalar.cc#L1239-L1250 The reason I am addressing these issues is fundamentally to remove the legacy `CastTo`. In the short term, there may be code that requires improvement, but I would like to approach this with the following steps: 1. Implement Cast Compute Kernel. - #41668 - #41669 - #41809 - #41810 2. Remove Legacy Cast. - #39182 3. Extend `StringFormatter` to support additional types (e.g., Union, Struct, List-Like, etc.). - New Issues. 4. Enable the use of `StringFormatter` within the Cast Compute Kernel. - New Issues. Even the existing implementation of `CastTo` (to string) does not utilize `StringFormatter` for complex types. For reference, please see: https://github.com/apache/arrow/blob/ff9921ffa89585be69ae85674bb365d03cb22ba4/cpp/src/arrow/scalar.cc#L1309-L1326 What do you think of this approach? I believe this is a way to address each issue and improve the code accordingly. Mentioning everyone who has reviewed the cast code. cc @kou, @felipecrv, @ZhangHuiGui -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1615425769
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
Also, this issue is related: https://github.com/apache/arrow/pull/41831.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1615425029
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
Thank you for the review and the insightful suggestions.
That makes sense, and I'll consider applying that.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
ZhangHuiGui commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1615404981
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
Maybe it's a better way to implement this by expand `StringFormatter`
(include other nested types)? @felipecrv
In this way, we can unify it with other type's implementations and shield
the logic of converting strings in the current file.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
ZhangHuiGui commented on code in PR #41827:
URL: https://github.com/apache/arrow/pull/41827#discussion_r1615404981
##
cpp/src/arrow/compute/kernels/scalar_cast_string.cc:
##
@@ -510,6 +510,60 @@ void AddBinaryToFixedSizeBinaryCast(CastFunction* func) {
AddBinaryToFixedSizeBinaryCast(func);
}
+// --
+// Union to String
+
+template
+struct UnionToStringCastFunctor {
+ using BuilderType = typename TypeTraits::BuilderType;
+
+ static Status Exec(KernelContext* ctx, const ExecSpan& batch, ExecResult*
out) {
+const ArraySpan& input = batch[0].array;
+const auto& union_type = checked_cast(*input.type);
+const auto type_ids = input.GetValues(1);
+const auto& offsets = input.GetValues(2);
+
+BuilderType builder(input.type->GetSharedPtr(), ctx->memory_pool());
+RETURN_NOT_OK(builder.Reserve(input.length));
+
+for (int64_t i = 0; i < input.length; ++i) {
Review Comment:
Maybe it's a better way to implement this by expand `StringFormatter`?
In this way, we can unify it with other type's implementations and shield
the logic of converting strings in the current file.
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
github-actions[bot] commented on PR #41827: URL: https://github.com/apache/arrow/pull/41827#issuecomment-2131238275 :warning: GitHub issue #41810 **has been automatically assigned in GitHub** to PR creator. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[PR] GH-41810: [C++] Support cast kernel from (dense or sparse) union to (large) string [arrow]
llama90 opened a new pull request, #41827: URL: https://github.com/apache/arrow/pull/41827 ### Rationale for this change Support `cast` kernel from (`dense` or `sparse`) union to (large) string ### What changes are included in this PR? - Support `cast` kernel - from `dense union` to (large) `string` - from `sparse union` to (large) `string` ### Are these changes tested? Yes. It is passed by existing test cases. ### Are there any user-facing changes? No. -- 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-unsubscr...@arrow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
