[GitHub] [arrow] westonpace commented on a diff in pull request #14495: ARROW-17989: [C++][Python] Enable struct_field kernel to accept string field names

[GitBox <gi...@apache.org>]

westonpace commented on code in PR #14495:
URL: https://github.com/apache/arrow/pull/14495#discussion_r1018538463


##########
cpp/src/arrow/engine/substrait/expression_internal.cc:
##########
@@ -1019,13 +1020,16 @@ Result<std::unique_ptr<substrait::Expression>> ToProto(
 
   if (call->function_name == "struct_field") {
     // catch the special case of calls convertible to a StructField
+    const auto& field_options =
+        checked_cast<const compute::StructFieldOptions&>(*call->options);
+    const DataType& struct_type = *call->arguments[0].type();
+    DCHECK_EQ(struct_type.id(), Type::STRUCT);

Review Comment:
   The `ToProto` path is very much "for debugging & testing purposes only" so I don't know that we need to consider too much the union case (e.g. the `DCHECK` is sufficient).  That being said, I opened https://github.com/substrait-io/substrait/issues/369 as it is an interesting idea to maybe support something like this in Substrait, which does have unions.
   
   If we want to broaden `ToProto` for use outside of unit tests then we'd want to return an invalid status instead of a `DCHECK` because users can make whatever crazy invalid call arguments they want (e.g. they could have multiple arguments as well which isn't checked here).
   
   We don't need to worry about integers vs. names though.  The field ref is provided to `FindOne` which should translate names into a field path (with indices).  So this should work even if the user supplies a field ref with names.
   
   In summary, +1 for the current implementation.



-- 
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