Skip to content

[Fix-17281][alert] Fix The maximum length of cell contents (text) is 32767 characters #17283

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
Merged
ruanwenjun merged 32 commits into from
Jul 8, 2025
Merged

[Fix-17281][alert] Fix The maximum length of cell contents (text) is 32767 characters #17283

ruanwenjun merged 32 commits into from
Jul 8, 2025

Conversation

@hkun0120
Copy link
Contributor

@hkun0120 hkun0120 commented Jun 25, 2025 •
edited by SbloodyS
Loading

Purpose of the pull request

fix #17281

Brief change log

Verify this pull request

This pull request is code cleanup without any test coverage.

(or)

This pull request is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(or)

Pull Request Notice

Pull Request Notice

If your pull request contains incompatible change, you should also add it to docs/docs/en/guide/upgrade/incompatible.md

@hkun0120 hkun0120 requested a review from SbloodyS as a code owner June 25, 2025 03:18
@SbloodyS SbloodyS added bug Something isn't working first time contributor First-time contributor labels Jun 25, 2025
@SbloodyS SbloodyS added this to the 3.3.1 milestone Jun 25, 2025
@github-actions github-actions bot added the test label Jun 26, 2025
hkun0120 added 7 commits June 26, 2025 10:42
@hkun0120
add ExcelUtilsTest
0dea739
@hkun0120
Merge branch 'dev' into dev
b82f6f6
@hkun0120
优化import
7a96187
@hkun0120
Merge remote-tracking branch 'origin/dev' into dev
e81a279 
# Conflicts:
#	dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-email/src/test/java/org/apache/dolphinscheduler/plugin/alert/email/ExcelUtilsTest.java
@hkun0120
优化import
50f42f1
@hkun0120
Merge remote-tracking branch 'origin/dev' into dev
b1d8ffe
@hkun0120
fix code style
249a771
@SbloodyS
Copy link
Member

SbloodyS commented Jun 26, 2025

You should run mvn spotless:apply to format code. @hkun0120

@hkun0120
Copy link
Contributor Author

hkun0120 commented Jun 26, 2025

@SbloodyS ./mvnw spotless:apply in my local, result is all success. My jdk in IDEA is openjdk20,is this the problem?

@SbloodyS
Copy link
Member

SbloodyS commented Jun 26, 2025

@SbloodyS ./mvnw spotless:apply in my local, result is all success. My jdk in IDEA is openjdk20,is this the problem?

JDK8 and JDK11 are supported right now.

@hkun0120
Copy link
Contributor Author

hkun0120 commented Jun 26, 2025

@SbloodyS ./mvnw spotless:apply in my local, result is all success. My jdk in IDEA is openjdk20,is this the problem?

JDK8 and JDK11 are supported right now.

@SbloodyS ./mvnw spotless:apply in my local, result is all success. My jdk in IDEA is openjdk20,is this the problem?

JDK8 and JDK11 are supported right now.

@SbloodyS okay，let's try again.

@SbloodyS SbloodyS mentioned this pull request Jun 26, 2025
Closed
@hkun0120
Copy link
Contributor Author

hkun0120 commented Jun 26, 2025

@SbloodyS why the CI can't load my ExcelUtilsTest.java ?

@hkun0120
Copy link
Contributor Author

hkun0120 commented Jun 26, 2025

@SbloodyS I think this is a problem in dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-email/src/test/java/org/apache/dolphinscheduler/plugin/alert/email/ExcelUtilsTest.java。Will I make another pull request to fix the testGenExcelFile() method?

@SbloodyS
Copy link
Member

SbloodyS commented Jun 27, 2025

@SbloodyS I think this is a problem in dolphinscheduler-alert/dolphinscheduler-alert-plugins/dolphinscheduler-alert-email/src/test/java/org/apache/dolphinscheduler/plugin/alert/email/ExcelUtilsTest.java。Will I make another pull request to fix the testGenExcelFile() method?

You should fix it in this PR since this is introduted by this.

@SbloodyS
Copy link
Member

SbloodyS commented Jul 1, 2025

we use SQL component,send our data in gauss database By Email plugin. For example, f1 field varchar in table t, the length of some f1 data beyond 32767 ,and then it happens

Then I understand that we should actually optimize the export function of sql nodes, not this alarm function.

@hkun0120
Copy link
Contributor Author

hkun0120 commented Jul 1, 2025 •
edited
Loading

we use SQL component,send our data in gauss database By Email plugin. For example, f1 field varchar in table t, the length of some f1 data beyond 32767 ,and then it happens

Then I understand that we should actually optimize the export function of sql nodes, not this alarm function.

Yes, I think it’s reasonable to modify the SQL component. can I/we create a DSIP for this problem ？And this PR just focuses on fixing the alert issue? @SbloodyS

@hkun0120 hkun0120 requested a review from SbloodyS July 2, 2025 01:21
@SbloodyS
Copy link
Member

SbloodyS commented Jul 2, 2025

can I/we create a DSIP for this problem?

You can create an improvement issue for a problem like this.

And this PR just focuses on fixing the alert issue?

Yes. I found that we will display multiple fields separately. If we split the extra-long fields into columns, it will lead to field dislocation, so we should intercept the columns. Sorry for my mistaken.

@hkun0120
Copy link
Contributor Author

hkun0120 commented Jul 2, 2025

can I/we create a DSIP for this problem?

You can create an improvement issue for a problem like this.

OK,I will create an improvement.
And this PR just focuses on fixing the alert issue?

Yes. I found that we will display multiple fields separately. If we split the extra-long fields into columns, it will lead to field dislocation, so we should intercept the columns. Sorry for my mistaken.

it doesn't matter. Shall I fix the code to the first edition, it means when length more than 32767, I change them to 32700 + '......terminated'?

@SbloodyS
Copy link
Member

SbloodyS commented Jul 2, 2025

Shall I fix the code to the first edition, it means when length more than 32767, I change them to 32700 + '......terminated'?

Yes.

hkun0120 added 3 commits July 2, 2025 21:50
@hkun0120
Update ExcelUtils.java
7b5c325 
terminate long string
@hkun0120
Update ExcelUtilsTest.java
30ddc0b 
add long str test
@hkun0120
Update ExcelUtilsTest.java
372fc18 
fix format
SbloodyS
SbloodyS requested changes Jul 3, 2025
View reviewed changes
@hkun0120 hkun0120 requested a review from SbloodyS July 4, 2025 06:10
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 8, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@hkun0120 hkun0120 requested review from pjfanning and ruanwenjun July 8, 2025 09:32
SbloodyS
SbloodyS approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@SbloodyS SbloodyS left a comment

Choose a reason for hiding this comment

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

LGTM

ruanwenjun
ruanwenjun approved these changes Jul 8, 2025
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

@ruanwenjun ruanwenjun merged commit 0b012c4 into apache:dev Jul 8, 2025
70 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Jul 8, 2025

Awesome work, congrats on your first merged pull request!

eco8848 pushed a commit to eco8848/dolphinscheduler that referenced this pull request Aug 8, 2025
@hkun0120 @HakukuMatata
[Fix-17281][alert] Fix the content exceed 32767 characters will throw…
c75b174 
… exception in excel (apache#17283)
davidzollo pushed a commit to davidzollo/dolphinscheduler that referenced this pull request Oct 27, 2025
@hkun0120 @davidzollo
[Fix-17281][alert] Fix the content exceed 32767 characters will throw…
a07f70d 
… exception in excel (apache#17283)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SbloodyS SbloodyS SbloodyS approved these changes

@ruanwenjun ruanwenjun ruanwenjun approved these changes

@Gallardot Gallardot Awaiting requested review from Gallardot

@pjfanning pjfanning Awaiting requested review from pjfanning

Assignees

@hkun0120 hkun0120

Labels

backend bug Something isn't working first time contributor First-time contributor test

Projects

None yet

Milestone

3.3.1

Development

Successfully merging this pull request may close these issues.

[Bug] [Alert] ExcelUtils The maximum length of cell contents (text) is 32767 characters

4 participants

@hkun0120 @SbloodyS @pjfanning @ruanwenjun