Skip to content

Conversation

@wgtmac
Copy link
Member

@wgtmac wgtmac commented Apr 23, 2025

No description provided.

@wgtmac wgtmac force-pushed the add_metadata_reader branch from cb375f1 to 096cd4c Compare April 23, 2025 04:23
@wgtmac wgtmac changed the title feat: add table metadata reader feat: add table metadata reader and writer Apr 23, 2025
Result<nlohmann::json> FromJsonString(const std::string& json_string) {
try {
return nlohmann::json::parse(json_string);
} catch (const std::exception& e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:should we only catch json::parse_error exception?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the official documentation says it only throws parse_error. I just want to be safe enough to catch all just in case. Or we can add a special branch for parse_error for better error message.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also call an overload that doesn't throw an exception, and explicitly check whether the resulting JSON object is discarded() (IIRC)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've created #87 to track it.

return false;
}
for (size_t i = 0; i < lhs.size(); ++i) {
if (*lhs[i] != *rhs[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be null pointer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be null if there is something wrong. Since operator== is only used in the test case, it should be fine. I can add a NullSafeEquals later if this is required.

Copy link
Collaborator

@zhjwpku zhjwpku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks

@Fokko Fokko merged commit a14c2b7 into apache:main Apr 23, 2025
6 checks passed
@Fokko
Copy link
Contributor

Fokko commented Apr 23, 2025

Thanks @wgtmac for working on this, and thanks @lidavidm, @zhjwpku and @yingcai-cy for the review 🙌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants