Skip to content

Conversation

MisterRaindrop
Copy link
Contributor

@MisterRaindrop MisterRaindrop commented Aug 22, 2025

avro schema add sanitize field name issue #192

@wgtmac
Copy link
Member

wgtmac commented Aug 23, 2025

Please add test cases. Thanks!

@MisterRaindrop
Copy link
Contributor Author

Please add test cases. Thanks!

Ok

@MisterRaindrop
Copy link
Contributor Author

MisterRaindrop commented Aug 24, 2025

@wgtmac Hi, cpp-linter report an error, maybe I forgot to open an issue cause it. Can you retry it? Thanks!

INFO:CPP Linter:0 clang-format-checks-failed
  INFO:CPP Linter:1 clang-tidy-checks-failed
  INFO:CPP Linter:1 checks-failed
  ERROR:CPP Linter:response returned 403 from POST https://api.github.com/repos/apache/iceberg-cpp/issues/190/comments with message: {"message":"Resource not accessible by integration","documentation_url":"https://docs.github.com/rest/issues/comments#create-an-issue-comment","status":"403"}
  Traceback (most recent call last):
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/bin/cpp-linter", line 8, in <module>
      sys.exit(main())
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/lib/python3.10/site-packages/cpp_linter/__init__.py", line 81, in main
      rest_api_client.post_feedback(files=files, args=args, clang_versions=clang_versions)
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/lib/python3.10/site-packages/cpp_linter/rest_api/github_api.py", line 264, in post_feedback
      self.update_comment(
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/lib/python3.10/site-packages/cpp_linter/rest_api/github_api.py", line 361, in update_comment
      self.api_request(url=comments_url, method=req_meth, data=payload)
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/lib/python3.10/site-packages/cpp_linter/rest_api/__init__.py", line 124, in api_request
      response.raise_for_status()
    File "/home/runner/work/_actions/cpp-linter/cpp-linter-action/f91c446a32ae3eb9f98fef8c9ed4c7cb613a4f8a/venv/lib/python3.10/site-packages/requests/models.py", line 1026, in raise_for_status
      raise HTTPError(http_error_msg, response=self)
  requests.exceptions.HTTPError: 403 Client Error: Forbidden for url: https://api.github.com/repos/apache/iceberg-cpp/issues/190/comments
  Error: Process completed with exit code 1.

@wgtmac
Copy link
Member

wgtmac commented Aug 24, 2025

image

You need to go to the files page to check specific linter errors.

@MisterRaindrop MisterRaindrop force-pushed the liuxiaoyu/sanitized_field_name branch from 8339497 to 51f205c Compare August 30, 2025 02:05
@wgtmac
Copy link
Member

wgtmac commented Sep 3, 2025

Let me know when it is ready to review. Thanks!

@MisterRaindrop
Copy link
Contributor Author

Let me know when it is ready to review. Thanks!

You can review thanks

@MisterRaindrop
Copy link
Contributor Author

@wgtmac You can review thanks

///
/// \param name The name to check.
/// \return True if the name is valid, false otherwise.
bool ValidAvroName(const std::string& name);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool ValidAvroName(const std::string& name);
bool ValidAvroName(std::string_view name);

std::string_view accepts wider types than std::string

Comment on lines +237 to +240
std::string origFieldName = std::string(sub_field.name());
bool isValidFieldName = ValidAvroName(origFieldName);
std::string fieldName =
isValidFieldName ? origFieldName : SanitizeFieldName(origFieldName);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
std::string origFieldName = std::string(sub_field.name());
bool isValidFieldName = ValidAvroName(origFieldName);
std::string fieldName =
isValidFieldName ? origFieldName : SanitizeFieldName(origFieldName);
bool is_valid_field_name = ValidAvroName(sub_field.name());
std::string field_name =
is_valid_field_name ? std::string(sub_field.name()) : SanitizeFieldName(sub_field.name());

Copy link
Member

Choose a reason for hiding this comment

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

We don't want to pay for a copy when it is a valid name.

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.

2 participants