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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
