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.

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

[improvement-15760][datasource-plugin] fix sql task split error #15760 #15794

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

songwenyong
Copy link
Contributor

@songwenyong songwenyong commented Apr 2, 2024

fix #15760

  • Fix the bug in SQL splitting by completing the task in two steps: 1. removeComment 2. split

  • Add a unit test for Hive SQL splitting.

Purpose of the pull request

Fix the SQL splitting bug in the datasource-plugin module so that the framework can correctly recognize the -- comments at the end of SQL lines.

Brief change log

Modify the DataSourceProcessor.splitAndRemoveComment method to make two calls. First, call SQLParserUtils.removeComment, and then call SQLParserUtils.split

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

* Fix the bug in SQL splitting by completing the task in two steps: 1. removeComment 2. split

* Add a unit test for Hive SQL splitting.
@mergeable mergeable bot removed the backend label Apr 2, 2024
@songwenyong songwenyong changed the title [Fix-15760][datasource-plugin] fix sql task split error (#15760) [Fix-15760][datasource-plugin] fix sql task split error #15760 Apr 2, 2024
@fuchanghai
Copy link
Member

fuchanghai commented Apr 2, 2024

hi @songwenyong cool,but i dont think this is bug ,i think this a feature, cc @caishunfeng @EricGao888 WDYT?

@SbloodyS
Copy link
Member

SbloodyS commented Apr 2, 2024

hi @songwenyong cool,but i dont think this is bug ,i think this a feature, cc @caishunfeng @EricGao888 WDYT?

It's more like an improvement for me.

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.

Please link this pr to your issue. @songwenyong

@SbloodyS SbloodyS added this to the 3.3.0 milestone Apr 2, 2024
@fuchanghai
Copy link
Member

hi @songwenyong cool,but i dont think this is bug ,i think this a feature, cc @caishunfeng @EricGao888 WDYT?

It's more like an improvement for me.

yep, you are right

@SbloodyS
Copy link
Member

SbloodyS commented Apr 3, 2024

Please change title to [Improvement-IssueId] and link this pr to issue. @songwenyong

@SbloodyS SbloodyS added the improvement make more easy to user or prompt friendly label Apr 3, 2024
@songwenyong
Copy link
Contributor Author

Please change title to [Improvement-IssueId] and link this pr to issue. @songwenyong

This PR resolves the existing issue #15760 which created by stevqin. Do I need to create a new Improvement issue?

Copy link
Contributor

@rickchengx rickchengx left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Apr 3, 2024

Codecov Report

Attention: Patch coverage is 22.22222% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 39.11%. Comparing base (c0435e5) to head (a3f0b57).

❗ Current head a3f0b57 differs from pull request most recent head 8aa4421. Consider uploading reports for the commit 8aa4421 to get more accurate results

Files Patch % Lines
...ce/api/datasource/AbstractDataSourceProcessor.java 0.00% 2 Missing ⚠️
...lickhouse/param/ClickHouseDataSourceProcessor.java 0.00% 2 Missing ⚠️
...source/dameng/param/DamengDataSourceProcessor.java 0.00% 2 Missing ⚠️
...n/datasource/db2/param/Db2DataSourceProcessor.java 0.00% 2 Missing ⚠️
...ostgresql/param/PostgreSQLDataSourceProcessor.java 0.00% 2 Missing ⚠️
.../sqlserver/param/SQLServerDataSourceProcessor.java 0.00% 2 Missing ⚠️
...tasource/trino/param/TrinoDataSourceProcessor.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                dev   #15794      +/-   ##
============================================
- Coverage     39.11%   39.11%   -0.01%     
  Complexity     4887     4887              
============================================
  Files          1326     1326              
  Lines         45206    45215       +9     
  Branches       4818     4818              
============================================
+ Hits          17683    17685       +2     
- Misses        25635    25641       +6     
- Partials       1888     1889       +1     

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

@fuchanghai fuchanghai changed the title [Fix-15760][datasource-plugin] fix sql task split error #15760 [improvement-15760][datasource-plugin] fix sql task split error #15760 Apr 3, 2024
Copy link
Member

@fuchanghai fuchanghai left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

sonarcloud bot commented Apr 3, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
20.0% Coverage on New Code (required ≥ 60%)

See analysis details on SonarCloud

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.

+1

@SbloodyS SbloodyS merged commit 5d8808d into apache:dev Apr 3, 2024
59 of 60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.3.0 backend first time contributor First-time contributor 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.

[Bug] [workflow] SQL type run as description
5 participants