sanjibansg commented on code in PR #12852:
URL: https://github.com/apache/arrow/pull/12852#discussion_r875294830
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -174,31 +134,51 @@ Result<ExtensionSet>
ExtensionSet::Make(std::vector<util::string_view> uris,
}
Result<ExtensionSet::TypeRecord> ExtensionSet::DecodeType(uint32_t anchor)
const {
- if (anchor >= types_.size() || types_[anchor].id.empty()) {
+ if (types_.find(anchor) == types_.end() || types_.at(anchor).id.empty()) {
return Status::Invalid("User defined type reference ", anchor,
" did not have a corresponding anchor in the
extension set");
}
- return types_[anchor];
+ return types_.at(anchor);
}
Result<uint32_t> ExtensionSet::EncodeType(const DataType& type) {
if (auto rec = registry_->GetType(type)) {
- return impl_->EncodeType(*rec, this);
+ RETURN_NOT_OK(this->AddUri(rec->id));
+ auto it_success =
+ types_map_.emplace(rec->id, static_cast<uint32_t>(types_map_.size()));
+ if (it_success.second) {
+ if (types_.find(static_cast<unsigned int>(types_.size())) !=
types_.end()) {
+ return Status::Invalid("Key already exist in the uris map");
+ }
Review Comment:
Using `DCHECK` now instead of only returning a Invalid status
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -174,31 +134,51 @@ Result<ExtensionSet>
ExtensionSet::Make(std::vector<util::string_view> uris,
}
Result<ExtensionSet::TypeRecord> ExtensionSet::DecodeType(uint32_t anchor)
const {
- if (anchor >= types_.size() || types_[anchor].id.empty()) {
+ if (types_.find(anchor) == types_.end() || types_.at(anchor).id.empty()) {
return Status::Invalid("User defined type reference ", anchor,
" did not have a corresponding anchor in the
extension set");
}
- return types_[anchor];
+ return types_.at(anchor);
}
Result<uint32_t> ExtensionSet::EncodeType(const DataType& type) {
if (auto rec = registry_->GetType(type)) {
- return impl_->EncodeType(*rec, this);
+ RETURN_NOT_OK(this->AddUri(rec->id));
+ auto it_success =
+ types_map_.emplace(rec->id, static_cast<uint32_t>(types_map_.size()));
+ if (it_success.second) {
+ if (types_.find(static_cast<unsigned int>(types_.size())) !=
types_.end()) {
+ return Status::Invalid("Key already exist in the uris map");
+ }
+ types_[static_cast<unsigned int>(types_.size())] = {rec->id, rec->type};
+ }
+ return it_success.first->second;
}
return Status::KeyError("type ", type.ToString(), " not found in the
registry");
}
Result<ExtensionSet::FunctionRecord> ExtensionSet::DecodeFunction(uint32_t
anchor) const {
- if (anchor >= functions_.size() || functions_[anchor].id.empty()) {
+ if (functions_.find(anchor) == functions_.end() ||
functions_.at(anchor).id.empty()) {
return Status::Invalid("User defined function reference ", anchor,
" did not have a corresponding anchor in the
extension set");
}
- return functions_[anchor];
+ return functions_.at(anchor);
}
Result<uint32_t> ExtensionSet::EncodeFunction(util::string_view function_name)
{
if (auto rec = registry_->GetFunction(function_name)) {
- return impl_->EncodeFunction(rec->id, rec->function_name, this);
+ RETURN_NOT_OK(this->AddUri(rec->id));
+ auto it_success =
+ functions_map_.emplace(rec->id,
static_cast<uint32_t>(functions_map_.size()));
+ if (it_success.second) {
+ if (functions_.find(static_cast<unsigned int>(functions_.size())) !=
+ functions_.end()) {
+ return Status::Invalid("Key already exist in the uris map");
+ }
Review Comment:
Made the change, thanks!
--
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]
