[yzeng1618]

Was this PR generated or assisted by AI?

YES. The code change, tests and PR description were generated with AI assistance (ZCode agent), under human review and verified locally.

Purpose of the pull request

Fix #18750

In the SQL task, the format of OUT parameters depended on how many rows the query returns:

result rows	LIST out param	non-LIST out param
> 1 (before)	["user2","user3"] (JSON array)	value never set (null)
= 1 (before)	user2 (bare string, not a valid JSON array)	the row value (with LONG normalization)

This made downstream tasks fail intermittently depending on the data: when a query happened to return exactly one row, a LIST out parameter was a bare string that cannot be parsed as a JSON array, and when a query returned multiple rows, non-LIST out parameters were never assigned (the null-valued property was still merged into the varPool). LONG normalization only applied to the single-row branch.

Brief change log

• SqlParameters#dealOutParam now collects column values for all rows in a single unified path instead of two row-count-dependent branches:
  • LIST OUT parameter always outputs a JSON array, e.g. ["user2"] for a single-row result
  • non-LIST OUT parameter takes the last row value (compatible with the previous single-row behaviour)
  • LONG normalization/validation applies to any row count (extracted into convertOutParamValue)
• Added incompatible-change entries to docs/docs/en/guide/upgrade/incompatible.md and docs/docs/zh/guide/upgrade/incompatible.md

Behavior changes (also documented in incompatible.md):

• single-row LIST output: user2 -> ["user2"]
• multi-row non-LIST output: null -> last row value

Verify this pull request

This change added tests and can be verified as follows:

• Added SqlParametersTest#testDealOutParamValueFormatShouldNotDependOnRowCount to verify LIST/non-LIST output for single-row and multi-row results
• Added SqlParametersTest#testDealOutParamWithNullValue to pin the existing null -> "null" string behaviour
• Added SqlParametersTest#testLongOutParameterNormalizesLastRowValueForMultiLineResult to verify LONG normalization on the multi-row path
• Ran locally: mvn test -pl dolphinscheduler-task-plugin/dolphinscheduler-task-api (SqlParametersTest: 20 passed) and mvn test -pl dolphinscheduler-task-plugin/dolphinscheduler-task-sql -am -Dtest=SqlTaskTest (38 passed)

[SbloodyS]

Do not silently select the last row for a non-LIST OUT parameter

[Evidence] The current documentation defines non-LIST OUT parameters for single-row results and LIST OUT parameters for multi-row results. However, this change silently selects the last row for a non-LIST parameter when multiple rows are returned.

[Evidence] Without an explicit ORDER BY, SQL result order is not guaranteed. Therefore, the “last row” is not deterministic and may pass a plausible but incorrect value to downstream tasks.

[Recommendation] When multiple rows are returned for a non-LIST OUT parameter, throw a TaskException containing the parameter name, type, and row count. The error should tell users to either use a LIST parameter or constrain the query to return one row.

[yzeng1618]

@SbloodyS Thanks for the review, good catch! Agreed on all three points:

• The docs do define non-LIST OUT parameters for single-row results only, so multi-row + non-LIST is a misconfiguration that should not be silently tolerated.
• Picking "the last row" is non-deterministic without ORDER BY and can pass a plausible but incorrect value downstream.
• The old behaviour (parameter silently left empty / null in the varPool) was not something any working pipeline could depend on, so failing fast has no real compatibility cost.

Adopted in 16ed11d: a non-LIST OUT parameter now throws a TaskException containing the parameter name, type and row count, e.g.

Invalid SQL OUT parameter 'varchar_user' of type VARCHAR: the query returned 2 rows, please use a LIST parameter or make the query return a single row(e.g. add LIMIT 1)

Also updated the tests (testDealOutParamValueFormatShouldNotDependOnRowCount, new testNonListOutParameterRejectsMultiRowResult) and the incompatible.md entries (en/zh) to describe the fail-fast behaviour. Issue #18750's expected-behavior section was updated accordingly.

[SbloodyS]

Preserve the real zero-row result instead of producing [""]

[Evidence] SqlTask#resultProcess converts an empty ResultSet into a synthetic row whose columns contain empty strings. That synthetic row is passed to dealOutParam, so a LIST OUT parameter for a query that returns zero rows becomes [""].

[Evidence] Zero rows and one row containing an empty string have different meanings. Passing [""] downstream may cause a task to process an artificial value that was never returned by the database.

[Evidence] The new tests call dealOutParam directly and therefore do not cover the production path through SqlTask#resultProcess.

[Recommendation] Keep the synthetic empty row only for alert rendering. OUT-parameter processing should receive the actual empty result. Define the zero-row behavior explicitly—preferably [] for a LIST parameter—and add a test covering the complete resultProcess to dealOutParam path.

[yzeng1618]

@SbloodyS Thanks again, another valid catch — adopted in 38b879b:

• SqlTask#resultProcess now serializes the real result for OUT-parameter processing and keeps the synthetic all-empty-string row only for the alert array (so the alert email still shows column headers for a zero-row result).
• Zero-row behaviour in dealOutParam is now explicit: a LIST OUT parameter gets []; a non-LIST OUT parameter throws TaskException (the synthetic "" value was never returned by the database, so failing fast has no compatibility cost — consistent with the multi-row decision).
• Tests: added SqlTaskTest#testZeroRowResultPassedToDealOutParam_IsRealEmptyResult covering the complete resultProcess -> dealOutParam path (LIST -> [], non-LIST -> TaskException), and updated the two existing testResultProcess_*EmptyResult tests which had locked the old synthetic-row behaviour (they now assert the real [] result). SqlParametersTest#testDealOutParamWithZeroRowResult covers the parameter-level behaviour.
• incompatible.md (en/zh) and issue [Bug] [task-sql] SQL task LIST OUT parameter format depends on the result row count #18750 updated with the zero-row behaviour.

All SqlParametersTest (21) and SqlTaskTest (39) pass locally.

[SbloodyS]

Do not serialize an unmatched LIST OUT parameter as JSON null

[Evidence] sqlResultFormat.get(info.getProp()) returns null when the configured OUT parameter name does not match any result column. The LIST branch then serializes that value directly, producing the string null instead of a JSON array.

[Evidence] The method subsequently merges every OUT property into the varPool, so the unmatched parameter is still propagated downstream with a non-array value. This violates the new guarantee that a LIST OUT parameter always contains a JSON array and can still break downstream LIST consumers.

[Evidence] The existing parameter documentation says that an OUT parameter with no matching result column should be discarded.

[Recommendation] Check whether the result column exists before handling its type. Either fail fast with a TaskException containing the unmatched parameter name and available result columns, or exclude the unmatched property from the varPool according to the documented behavior. Do not serialize a missing column as JSON null.

[yzeng1618]

@SbloodyS Good catch on the unmatched-name path too — adopted in ace2234:

• The LIST branch no longer serializes a missing column: before the type branch, an OUT parameter (LIST or not) whose name matches no result column now throws a TaskException containing the parameter name and the available result columns, e.g.

Invalid SQL OUT parameter 'varchar_user': no matching column in the query result(available columns: [user]), please make the OUT parameter name consistent with the result column name

• I went with fail fast over silently discarding the property, as it directly surfaces typos in the parameter name; the discarded-property option would leave downstream references to an undefined variable, which is harder to debug. Both options satisfy the docs contract, happy to switch if you prefer discard.
• Tests: SqlParametersTest#testUnmatchedOutParameterNameFailsFast covers LIST and non-LIST unmatched names (22 tests pass); SqlTaskTest (39) still green. incompatible.md (en/zh) and issue [Bug] [task-sql] SQL task LIST OUT parameter format depends on the result row count #18750 updated.

[SbloodyS]

Validate LIST OUT parameter names for zero-row results

[Evidence] In the zero-row branch, every LIST OUT parameter is assigned [] before any result-column validation occurs. Because resultProcess serializes an empty result as [], dealOutParam has already lost the column names from ResultSetMetaData.

[Impact] A misspelled LIST parameter therefore succeeds when the query returns zero rows, but the same configuration fails when rows later appear. The promised fail-fast behavior still depends on the result row count.

[Recommendation] Validate OUT parameter names against ResultSetMetaData while the schema is still available, or preserve the column schema alongside the rows passed to dealOutParam.

---

Update the parameter contract documentation

[Evidence] The new implementation and incompatible-change note say that an unmatched OUT parameter fails the task. However, both docs/docs/en/contribute/backend/mechanism/global-parameter.md and its Chinese counterpart still say that unmatched parameters are discarded.

[Impact] The repository now documents two different contracts for the same behavior.

[Recommendation] Update both global-parameter documents to describe the new fail-fast behavior.

---

[Remove the dialect-specific and ineffective LIMIT 1 advice

[Evidence] The same error message is used for both zero-row and multi-row results. Adding LIMIT 1 cannot fix a zero-row result, and DolphinScheduler supports databases such as Oracle and SQL Server where that syntax is not portable.

[Recommendation] Use dialect-neutral wording such as: “adjust the query so that it returns exactly one row.”

[yzeng1618]

@SbloodyS All three points adopted in 02edcb3:

1. Zero-row LIST name validation: SqlTask#resultProcess now calls the new SqlParameters#checkOutParamColumns(columnLabels) in the zero-row branch, while the ResultSetMetaData schema is still available — a misspelled LIST parameter fails there instead of succeeding as [] until rows appear. dealOutParam itself keeps assigning [] on zero rows (it has no column info left), so the production path is covered by SqlTaskTest#testZeroRowResultValidatesOutParamNamesAgainstSchema (misspelled idd vs column id -> TaskException).

2. Docs: global-parameter.md (en + zh) now describes the fail-fast contract instead of the old “discard if unmatched” wording, consistent with incompatible.md.

3. Error message: dropped the LIMIT 1 advice, the message now ends with “please use a LIST parameter or adjust the query so that it returns exactly one row”.

SqlParametersTest (22) and SqlTaskTest (40) pass locally. Issue #18750 updated.

[SbloodyS]

LGTM

[ruanwenjun]

Suggested change

	public void checkOutParamColumns(String[] columnLabels) {
	List<Property> outProperty = getOutProperty(localParams);
	if (CollectionUtils.isEmpty(outProperty) || ArrayUtils.isEmpty(columnLabels)) {
	return;
	}
	Set<String> columns = new HashSet<>(Arrays.asList(columnLabels));
	for (Property info : outProperty) {
	if (!columns.contains(info.getProp())) {
	throw unmatchedOutParamColumn(info, columns);
	}
	}
	}
	public void checkOutParamColumns(String[] columnLabels) {
	List<Property> outProperty = getOutProperty(localParams);
	if (CollectionUtils.isEmpty(outProperty)) {
	return;
	}
	if (ArrayUtils.isEmpty(columnLabels)) {
	throw xxException()
	}
	Set<String> columns = new HashSet<>(Arrays.asList(columnLabels));
	for (Property info : outProperty) {
	if (!columns.contains(info.getProp())) {
	throw unmatchedOutParamColumn(info, columns);
	}
	}
	}

[ruanwenjun]

Need to throw exception here?

[yzeng1618]

@ruanwenjun Thanks for the review! You are right — with OUT parameters to validate, missing column information can never match, so silently skipping the validation was a hole in the fail-fast contract. Adopted in c99f36b: checkOutParamColumns now treats an empty label array as an empty column set, so the first OUT parameter fails with the standard unmatched-column error (listing available columns: []), e.g.

Invalid SQL OUT parameter 'list_user': no matching column in the query result(available columns: []), please make the OUT parameter name consistent with the result column name

Covered by SqlParametersTest#testCheckOutParamColumnsRejectsMissingColumnInformation (23 tests pass; SqlTaskTest 40).

[ruanwenjun]

LGTM

[ruanwenjun]

@yzeng1618 Please fix the format issue.