westonpace commented on code in PR #12852:
URL: https://github.com/apache/arrow/pull/12852#discussion_r856547712
##########
cpp/src/arrow/engine/substrait/plan_internal.cc:
##########
@@ -108,13 +108,23 @@ void SetElement(size_t i, const Element& element,
std::vector<T>* vector) {
}
(*vector)[i] = static_cast<T>(element);
}
+
+template <typename Element, typename key, typename value>
+void SetMapElement(key i, const Element& element, std::unordered_map<key,
value>* map) {
+ DCHECK_LE(i, 1 << 20);
+ if (i >= map->size()) {
+ map->reserve(i + 1);
+ }
+ (*map)[i] = static_cast<value>(element);
+}
+
} // namespace
Result<ExtensionSet> GetExtensionSetFromPlan(const substrait::Plan& plan,
ExtensionIdRegistry* registry) {
- std::vector<util::string_view> uris;
+ std::unordered_map<uint32_t, util::string_view> uris;
for (const auto& uri : plan.extension_uris()) {
- SetElement(uri.extension_uri_anchor(), uri.uri(), &uris);
+ SetMapElement(uri.extension_uri_anchor(), uri.uri(), &uris);
}
// NOTE: it's acceptable to use views to memory owned by plan;
ExtensionSet::Make
Review Comment:
> That doesn't work, because there's no requirement for the indices to be
sequential for incoming plans
If it's an incoming plan we don't need to invent anchors. This "come up
with a new anchor to use" problem only occurs when going from Arrow ->
Substrait. I don't think it makes sense to share an ExtensionSet between
production and consumption scenarios.
Thanks for the explanation of variations. What you are describing makes
sense. It would allow functions to restrict themselves to a certain "class" of
a type. I'd be curious to see a real world use case for such a thing but
otherwise I agree that consumers can safely ignore them (provided they aren't
expected to be validating consumers)
--
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]
