Skip to content

[AMORO-3464] Add group name format validation.#3467

Closed
CosmosNi wants to merge 2 commits intoapache:masterfrom
CosmosNi:implement_paimon_code_style
Closed

[AMORO-3464] Add group name format validation.#3467
CosmosNi wants to merge 2 commits intoapache:masterfrom
CosmosNi:implement_paimon_code_style

Conversation

@CosmosNi
Copy link
Contributor

@CosmosNi CosmosNi commented Mar 13, 2025

Why are the changes needed?

Close #3464.

Brief change log

How was this patch tested?

  • Add some test cases that check the changes thoroughly including negative and positive cases if possible

  • Add screenshots for manual tests if appropriate

  • Run test locally before making a pull request

Documentation

  • Does this pull request introduce a new feature? (yes / no)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

@github-actions github-actions bot added the module:ams-server Ams server module label Mar 13, 2025
@CosmosNi CosmosNi changed the title 【paimon-format】keep code style [AMORO-3464] Add group name format validation. Mar 13, 2025
Copy link
Member

@klion26 klion26 left a comment

Choose a reason for hiding this comment

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

@CosmosNi thanks for the contribution, LGTM, and thanks for separating the different changes into different commits, that makes the history more clear.
two minor things: 1) could you please change the commit message of the second commit to [hotfix] and keep using English characters. 2) do you mind add some test case for this change? thanks.

@CosmosNi
Copy link
Contributor Author

@CosmosNi thanks for the contribution, LGTM, and thanks for separating the different changes into different commits, that makes the history more clear. two minor things: 1) could you please change the commit message of the second commit to [hotfix] and keep using English characters. 2) do you mind add some test case for this change? thanks.
Thanks for reply,I'll fix it now.

@CosmosNi CosmosNi requested a review from klion26 March 14, 2025 05:39
@klion26 klion26 force-pushed the implement_paimon_code_style branch from cb51fe0 to 31de5f3 Compare March 14, 2025 06:13
Copy link
Member

@klion26 klion26 left a comment

Choose a reason for hiding this comment

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

@CosmosNi thanks for the quick fix. LGTM. I've arranged some of the code, squash the code format improving commits into a single one. Please have a double look when you're free. thanks.

String container = (String) map.get("container");
Map<String, String> properties = (Map) map.get("properties");

if (!ValidatorUtil.validateGroupName(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a extra util class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get

// use String
return StringType$.MODULE$;
case FIXED:
return BinaryType$.MODULE$;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a other minor code simplfy, I think we should raise another PR to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get

@CosmosNi
Copy link
Contributor Author

@XBaith @klion26 Split two pr, #3473 and #3471 ,PTAL

@CosmosNi CosmosNi closed this Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

module:ams-server Ams server module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Improvement]: Add group name format validation

3 participants