Repository navigation
[Improvement-18714][Task-Sql] Avoid redundant serialization in alert content limiting and log result rows incrementally in query result processing in SqlTask - #18717
Open
njnu-seafish wants to merge 2 commits into
Conversation
njnu-seafish
requested review from
Gallardot,
SbloodyS and
caishunfeng
as code owners
October 16, 2026 11:14
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Was this PR generated or assisted by AI?
YES.
Purpose of the pull request
close #18714 .
Improve the SQL task's query processing in two aspects:
emit the display-row logs while the result set is being fetched instead of after the whole result is buffered,
and build the size-limited query-result alert without re-serializing the JSON on every dropped row.
Brief change log
Log result rows incrementally while fetching (
SqlTask.resultProcess)display the first {N} rows of sql result as follows:header, per-rowrow {k} : ...lines, and a trailingdisplayed {X} of {Y} rowssummary), replacing the previous behavior where all rows were buffered first and then logged in a second pass. This gives real-time visibility for long-running / high-volume queries and removes the extra traversal of the result array.displayRows.Avoid redundant serialization when limiting the alert content (
SqlTask.limitAlertContent)arithmetic, and the final content is assembled once. The cost drops from O(dropped-rows × rows) to O(rows), with the serialized output unchanged (still a valid JSON array respecting the configured max-rows / value-length / content-bytes budgets).
Test plumbing (
dolphinscheduler-task-sql/pom.xml)slf4j-reload4jfrom the module'sdolphinscheduler-datasource-apideclaration so logback stays the only SLF4J binding on the test classpath, making the new log-output assertions capturable through the logback API. Scoped to this module only; no runtime impact (provided scope). See the dedicated section below.Verify this pull request
add unit test cases.
SqlTaskTest: 38 tests pass, including new contract tests for the incremental log output and exact byte-boundary of the alert content (mutation-verified).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