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

[#3088] Improvement(jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter #3120

Merged
merged 3 commits into from
Jun 3, 2024
Merged

[#3088] Improvement(jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter #3120

merged 3 commits into from
Jun 3, 2024

Conversation

BSSsunny
Copy link
Contributor

@BSSsunny BSSsunny commented Apr 23, 2024 •
edited
Loading

What changes were proposed in this pull request?

#3088

Why are the changes needed?

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

Does this PR introduce any user-facing change?

No

How was this patch tested?

IT&UT

@jerryshao
Copy link
Collaborator

@yuqi1129 @zhoukangcn would you please help to review this?

@zhoukangcn
Copy link
Contributor

LGTM

@yuqi1129
Copy link
Contributor

yuqi1129 commented May 7, 2024

@BSSsunny
Could you update your PR description to align with that of other PRs?

yuqi1129
yuqi1129 approved these changes May 7, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 left a comment

Choose a reason for hiding this comment

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

LGTM

@BSSsunny
Copy link
Contributor Author

BSSsunny commented May 7, 2024 •
edited by yuqi1129
Loading

@yuqi1129
[Improvement]（catalogs） Improve greedy regular expressions.
Is it right ?I'm sorry,this is my first pr.

You can change it to '[Improvement] (jdbc-doris): Improve greedy regular expressions'

The main point I that I mean is that you need to change this part:
image

@zhoukangcn
Copy link
Contributor

zhoukangcn commented May 8, 2024 •
edited
Loading

PR's Title should be : [#3088]improvement(jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter

and Content should be

### What changes were proposed in this pull request?
Improve greedy regular expression in DorisExceptionConverter.java to xxxxx

### Why are the changes needed?
Fix: #3088

Does this PR introduce any user-facing change?
N/A

How was this patch tested?
UT & IT

@BSSsunny BSSsunny changed the title [Improvement] Improve greedy regular expressions #3088 [Improvement] (jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter #3088 May 8, 2024
@BSSsunny
Copy link
Contributor Author

BSSsunny commented May 8, 2024

Thanks for your help,I have changed it.

@yuqi1129
Copy link
Contributor

yuqi1129 commented May 8, 2024

Thanks for your help,I have changed it.

Have you altered this aspect?
image

@BSSsunny
Copy link
Contributor Author

BSSsunny commented May 8, 2024

谢谢你的帮助，我已经改变了它。

你改变了这方面吗？ image

I'm sorry.Maybe it didn't take effect when I modified it. I checked it again and modified it.

@yuqi1129
Copy link
Contributor

yuqi1129 commented May 8, 2024

谢谢你的帮助，我已经改变了它。

你改变了这方面吗？ image

I'm sorry.Maybe it didn't take effect when I modified it. I checked it again and modified it.

Can you change UT & IT to the Existing test can cover it or you can add some tests to verify your changes?

@BSSsunny
Copy link
Contributor Author

BSSsunny commented May 8, 2024 •
edited
Loading

谢谢你的帮助，我已经改变了它。

你改变了这方面吗？ image

I'm sorry.Maybe it didn't take effect when I modified it. I checked it again and modified it.

Can you change to the or you can add some tests to verify your changes?UT & IT``Existing test can cover it

Sorry maybe I should take time to add tests for this change.

This was referenced May 10, 2024
Merged
Open
@jerryshao
Copy link
Collaborator

@BSSsunny can you please move forward this?

@BSSsunny
Copy link
Contributor Author

你能推进这个吗？

I'm sorry l've been so busy with exams and interviews that l don't have the energy to do it

@mchades mchades changed the title [Improvement] (jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter #3088 [#3088] Improvement(jdbc-doris): Improve greedy regular expressions for DorisExceptionConverter Jun 3, 2024
@mchades mchades merged commit 863d42d into datastrato:main Jun 3, 2024
22 checks passed
@mchades
Copy link
Contributor

mchades commented Jun 3, 2024

Thank you for your contributions! @BSSsunny

diqiu50 pushed a commit to diqiu50/gravitino that referenced this pull request Jun 13, 2024
@BSSsunny @yuqi1129 @diqiu50
[datastrato#3088] Improvement(jdbc-doris): Improve greedy regular exp…
d3971a3 
…ressions for DorisExceptionConverter (datastrato#3120)

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

datastrato#3088

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

Fix: datastrato#3088

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

Co-authored-by: Qi Yu <yuqi@datastrato.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuqi1129 yuqi1129 yuqi1129 approved these changes

Assignees

@BSSsunny BSSsunny

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@BSSsunny @jerryshao @zhoukangcn @yuqi1129 @mchades