Skip to content

Conversation

@ruanwenjun
Copy link
Member

@ruanwenjun ruanwenjun commented Dec 26, 2023

Purpose of the pull request

Right now, we use ;\n to split the sql and use simple way to remove the comments. This cannot work will if the sql is complex, this PR will use Druid to split sql and remove comments.

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)

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

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch 3 times, most recently from 4720539 to 034c483 Compare December 26, 2023 12:38
@ruanwenjun ruanwenjun added the improvement make more easy to user or prompt friendly label Dec 26, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch 4 times, most recently from 6921e9a to 550d1c6 Compare December 26, 2023 13:44
caishunfeng
caishunfeng previously approved these changes Dec 27, 2023
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM

}

@Override
public List<String> splitAndRemoveComment(String sql) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some new UT for it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Add some UT case for Oracle, Dorid, Mysql.

@caishunfeng caishunfeng changed the title Use Druid to split multiple sql [Improvement] Use Druid to split multiple sql Dec 27, 2023
@caishunfeng
Copy link
Contributor

Please add some pr description.

@ruanwenjun
Copy link
Member Author

Please add some pr description.

Done

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch from f2ddfa0 to 79b6bce Compare December 27, 2023 03:25
caishunfeng
caishunfeng previously approved these changes Dec 27, 2023
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

+1

@caishunfeng caishunfeng added this to the 3.2.1 milestone Dec 27, 2023
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch 2 times, most recently from 56ec086 to 9c3425a Compare December 27, 2023 10:28
@codecov-commenter
Copy link

codecov-commenter commented Dec 27, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (c3ffb13) 38.11% compared to head (0f8bb07) 38.11%.

❗ Current head 0f8bb07 differs from pull request most recent head e27bb5c. Consider uploading reports for the commit e27bb5c to get more accurate results

Files Patch % Lines
...ache/dolphinscheduler/plugin/task/sql/SqlTask.java 0.00% 2 Missing ⚠️
...ce/api/datasource/AbstractDataSourceProcessor.java 0.00% 1 Missing ⚠️
...lickhouse/param/ClickHouseDataSourceProcessor.java 0.00% 1 Missing ⚠️
...source/dameng/param/DamengDataSourceProcessor.java 0.00% 1 Missing ⚠️
...n/datasource/db2/param/Db2DataSourceProcessor.java 0.00% 1 Missing ⚠️
...datasource/hive/param/HiveDataSourceProcessor.java 0.00% 1 Missing ⚠️
...ostgresql/param/PostgreSQLDataSourceProcessor.java 0.00% 1 Missing ⚠️
.../sqlserver/param/SQLServerDataSourceProcessor.java 0.00% 1 Missing ⚠️
...tasource/trino/param/TrinoDataSourceProcessor.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15367      +/-   ##
============================================
- Coverage     38.11%   38.11%   -0.01%     
+ Complexity     4698     4697       -1     
============================================
  Files          1300     1299       -1     
  Lines         44778    44783       +5     
  Branches       4800     4798       -2     
============================================
- Hits          17069    17068       -1     
- Misses        25858    25864       +6     
  Partials       1851     1851              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch 4 times, most recently from 5066043 to 8532464 Compare December 28, 2023 07:30
caishunfeng
caishunfeng previously approved these changes Dec 28, 2023
Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

+1

@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch from 8532464 to 62b6b5b Compare January 2, 2024 02:23
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch 2 times, most recently from d916888 to d8e1045 Compare January 2, 2024 02:52
@ruanwenjun ruanwenjun force-pushed the dev_wenjun_useDruidParseSqlDev branch from d8e1045 to e27bb5c Compare January 2, 2024 02:56
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

42.1% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

Copy link
Contributor

@caishunfeng caishunfeng left a comment

Choose a reason for hiding this comment

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

LGTM again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend improvement make more easy to user or prompt friendly ready-to-merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants