Skip to content

[Fix-17040][sql] Preserve PL/pgSQL DO $$ blocks in PostgreSQL SQL splitting #17145

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
SbloodyS merged 1 commit into from
Apr 23, 2025
Merged

[Fix-17040][sql] Preserve PL/pgSQL DO $$ blocks in PostgreSQL SQL splitting #17145

SbloodyS merged 1 commit into from
Apr 23, 2025

Conversation

@DhiyaneshwaranR
Copy link
Contributor

@DhiyaneshwaranR DhiyaneshwaranR commented Apr 18, 2025

Purpose of the pull request

Fixes issue #17040

This PR fixes incorrect splitting of PL/pgSQL DO $$...$$; blocks in PostgreSQL SQL tasks.

Root cause

The regression was introduced in #15367, which enabled Druid-based SQL splitting.
However, Druid's default SQL parser splits on semicolons — including those inside dollar-quoted DO $$...$$ blocks — which breaks valid PL/pgSQL scripts by triggering errors like Unterminated dollar quote.

Brief change log

  • Override splitAndRemoveComment() in PostgreSQLDataSourceProcessor
  • Implement custom dollar-quote-aware SQL splitter
  • Preserve semicolons inside PL/pgSQL DO $$ ... $$ blocks
  • Leave standard SQL splitting behaviour unchanged
  • Add unit tests to verify proper behaviour for multi-statement scripts

Verify this pull request

This change added tests and can be verified as follows:

  • ✅ Manually tested using DolphinScheduler UI SQL task with:
    • Single DO $$ ... $$; block
    • DO block followed by INSERT and UPDATE
  • ✅ Added unit tests in PostgreSQLDataSourceProcessorTest:
    • DO block only
    • DO block + INSERT
    • Dollar-tagged blocks ($func$)
    • Standard SQL with multiple statements

Documentation

This pull request does not require documentation update — it only fixes internal SQL splitting logic for PostgreSQL.

Incompatible Changes

This PR does not introduce any incompatible changes.

@boring-cyborg
Copy link

boring-cyborg bot commented Apr 18, 2025

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

@DhiyaneshwaranR
Copy link
Contributor Author

DhiyaneshwaranR commented Apr 18, 2025

Hi @chaz6chez @SbloodyS 👋

This PR addresses issue #17040 by correctly handling PostgreSQL DO $$...$$; blocks that were being split incorrectly by Druid.

  • It introduces a dollar-quote-aware SQL splitter in PostgreSQLDataSourceProcessor
  • Semicolons inside PL/pgSQL blocks are now preserved properly
  • Regular multi-statement SQL still works as expected
  • Unit tests and manual verification via the UI have been added ✅

Please have a look when you get time. Would love your feedback 🙌

@SbloodyS SbloodyS added the bug Something isn't working label Apr 19, 2025
@SbloodyS SbloodyS added this to the 3.3.1 milestone Apr 19, 2025
@DhiyaneshwaranR DhiyaneshwaranR force-pushed the fix-17040-plpgsql-split branch from 4676e86 to e837391 Compare April 19, 2025 09:07
@DhiyaneshwaranR
Copy link
Contributor Author

DhiyaneshwaranR commented Apr 19, 2025

Hi @SbloodyS, I’ve pushed the latest changes with spotless and license fixes — all set now.
Would be great if you could take a quick look when you're free.
Thanks again!

@SbloodyS
Copy link
Member

SbloodyS commented Apr 19, 2025

Hi @SbloodyS, I’ve pushed the latest changes with spotless and license fixes — all set now. Would be great if you could take a quick look when you're free. Thanks again!

I'm not quite familiar with postgresql. PTAL @Gallardot @ruanwenjun @caishunfeng

ruanwenjun
ruanwenjun reviewed Apr 21, 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.

Thanks for your PR.

@DhiyaneshwaranR DhiyaneshwaranR force-pushed the fix-17040-plpgsql-split branch from e837391 to 1492558 Compare April 21, 2025 02:32
@DhiyaneshwaranR
Copy link
Contributor Author

DhiyaneshwaranR commented Apr 21, 2025

Hi @ruanwenjun and @SbloodyS — I’ve pushed the latest changes to address the review feedback.
Let me know if anything else needs attention. Hopefully this finalises everything in the PR.
Appreciate your time and support!

@nielifeng nielifeng requested a review from Copilot April 21, 2025 07:07
Copilot AI reviewed Apr 21, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the incorrect splitting of PL/pgSQL DO $$ blocks in PostgreSQL SQL tasks. The changes include overriding the splitAndRemoveComment() method with a custom splitter that preserves dollar-quoted blocks and adding unit tests to verify the behavior.

  • Override splitAndRemoveComment() in PostgreSQLDataSourceProcessor to use a custom splitter.
  • Implement a dollar-quote-aware SQL splitting algorithm.
  • Add comprehensive unit tests for various PL/pgSQL and standard SQL scripts.

Reviewed Changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 1 comment.

File Description
dolphinscheduler-datasource-plugin/dolphinscheduler-datasource-postgresql/src/test/java/org/apache/dolphinscheduler/plugin/datasource/postgresql/PostgreSQLDataSourceProcessorTest.java Added unit tests for DO block and standard SQL splitting scenarios
dolphinscheduler-datasource-plugin/dolphinscheduler-datasource-postgresql/src/main/java/org/apache/dolphinscheduler/plugin/datasource/postgresql/param/PostgreSQLDataSourceProcessor.java Replaces Druid-based SQL splitting with a custom implementation that preserves dollar-quoted blocks
Files not reviewed (1)
  • .idea/vcs.xml: Language not supported

ruanwenjun
ruanwenjun reviewed Apr 21, 2025
View reviewed changes
@DhiyaneshwaranR DhiyaneshwaranR force-pushed the fix-17040-plpgsql-split branch from 1492558 to af8f71c Compare April 21, 2025 13:38
@DhiyaneshwaranR DhiyaneshwaranR force-pushed the fix-17040-plpgsql-split branch from af8f71c to c426b95 Compare April 21, 2025 13:41
@DhiyaneshwaranR
Copy link
Contributor Author

DhiyaneshwaranR commented Apr 21, 2025

Hi @ruanwenjun, I’ve updated the logic based on a suggestion from Copilot and adjusted the test cases as per your feedback. I’ve also added a few more test cases to cover additional edge scenarios.

Hopefully, everything looks good now and this finalises the PR.
Thanks again for your support and helpful feedback!

ruanwenjun
ruanwenjun approved these changes Apr 22, 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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 22, 2025

Quality Gate Passed Quality Gate passed

Issues
16 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

SbloodyS
SbloodyS approved these changes Apr 23, 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.

+1

@SbloodyS SbloodyS merged commit 1caa65d into apache:dev Apr 23, 2025
70 checks passed
@boring-cyborg
Copy link

boring-cyborg bot commented Apr 23, 2025

Awesome work, congrats on your first merged pull request!

davidzollo pushed a commit to davidzollo/dolphinscheduler that referenced this pull request Oct 27, 2025
@DhiyaneshwaranR @davidzollo
[Fix][SQL][apache#17040] Preserve PostgreSQL PL/pgSQL DO $$ blocks du…
731e85d 
…ring SQL splitting (apache#17145)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@SbloodyS SbloodyS SbloodyS approved these changes

@ruanwenjun ruanwenjun ruanwenjun approved these changes

Assignees

@DhiyaneshwaranR DhiyaneshwaranR

Labels

backend bug Something isn't working priority:middle test

Projects

None yet

Milestone

3.3.1

Development

Successfully merging this pull request may close these issues.

3 participants

@DhiyaneshwaranR @SbloodyS @ruanwenjun