Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Improvement] Improve greedy regular expressions #3088

Closed
justinmclean opened this issue Apr 22, 2024 · 4 comments · May be fixed by #3399
Closed

[Improvement] Improve greedy regular expressions #3088

justinmclean opened this issue Apr 22, 2024 · 4 comments · May be fixed by #3399
Assignees
@BSSsunny
Labels
good first issue Good for newcomers improvement Improvements on everything
Milestone
Gravitino June Re...

Comments

@justinmclean
Copy link
Member

What would you like to be improved?

In DorisExceptionConverter.java we have a greedy regular expression that could potentially cause issues.

  private static final String DATABASE_ALREADY_EXISTS_PATTERN_STRING =
      ".*detailMessage = Can't create database '.*'; database exists";

How should we improve?

Change the regular expression to not be greedy or don't use a regular expression.
@justinmclean justinmclean added good first issue Good for newcomers improvement Improvements on everything labels Apr 22, 2024
BSSsunny added a commit to BSSsunny/gravitino that referenced this issue Apr 23, 2024
@BSSsunny
[Improvement] Improve greedy regular expressions apache#3088
9d68486
@BSSsunny BSSsunny mentioned this issue Apr 23, 2024
Merged
@BSSsunny
Copy link
Contributor

How about change it to
private static final String DATABASE_ALREADY_EXISTS_PATTERN_STRING = ".*?detailMessage = Can't create database '.*?'; database exists";

@jerryshao jerryshao added this to the Gravitino June Release milestone Apr 24, 2024
@kiratkumar47
Copy link
Contributor

This variable is solely utilized within the getErrorCodeFromMessage(String message) method. Considering that SQLExceptions return error codes such as 1007 for a missing database and 1051 for a missing table, it might be beneficial to refactor the process to accept error codes instead of error messages. This adjustment could enhance its efficiency and clarity. Please let me know if there are any oversights since I'm eager to collaborate on this.

@justinmclean
Copy link
Member Author

I think that sounds like a better approach. If you have time, try it out and see what needs to change.

@kiratkumar47
Copy link
Contributor

Hi @justinmclean
#3399
This PR is ready for review.

@kiratkumar47 kiratkumar47 mentioned this issue May 15, 2024
Open
@mchades mchades closed this as completed in 863d42d Jun 3, 2024
@jerryshao jerryshao added the 0.6.0 label Jun 3, 2024
diqiu50 pushed a commit to diqiu50/gravitino that referenced this issue Jun 13, 2024
@BSSsunny @yuqi1129 @diqiu50
[apache#3088] Improvement(jdbc-doris): Improve greedy regular express…
d3971a3 
…ions for DorisExceptionConverter (apache#3120)

### What changes were proposed in this pull request?

apache#3088

### Why are the changes needed?
In DorisExceptionConverter.java we have a greedy regular expression that
could potentially cause issues

Fix: apache#3088

### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
 IT&UT

Co-authored-by: Qi Yu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@BSSsunny BSSsunny

Labels
good first issue Good for newcomers improvement Improvements on everything
Projects
None yet
Milestone
Gravitino June Release
Development

Successfully merging a pull request may close this issue.

[#3088] Improve greedy regular expressions kiratkumar47/gravitino
4 participants
@justinmclean @jerryshao @kiratkumar47 @BSSsunny