[njnu-seafish]

Was this PR generated or assisted by AI?

NO

Purpose of the pull request

close #18702

Brief change log

Make SQL task default query row limit configurable via common.properties

Verify this pull request

add unit tests

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

[njnu-seafish]

Hi @SbloodyS , I'd like to get your opinion on the limit field in SqlParameters.

limit is the task-level max-rows override for query SQL, but it has no form field in the web UI — there is no reference to it anywhere in dolphinscheduler-ui, so it can only be set through the taskParams JSON via the API.

In this PR it is still honored: when positive it takes precedence over the new sql.task.query.max.rows property, otherwise we fall back to it. Since it is invisible in the UI, I documented its semantics and precedence in the field javadoc (52e187d) rather than changing the behavior.

Given it is UI-less, do you think it is worth keeping at all? Two options:

1. Keep it as a documented API-only escape hatch (current state).
2. Remove it and leave sql.task.query.max.rows as the only knob. Deserialization stays safe (unknown properties in saved taskParams are ignored), but existing definitions that pass limit via the API would silently lose the per-task override.

I'm fine with either. If you prefer removal, I can do it in this PR or as a follow-up issue/PR, whichever keeps the scope cleaner.

[SbloodyS]

Please provide this parameter to the user in the UI of the task definition, which I think is meaningless here.

[njnu-seafish]

Please provide this parameter to the user in the UI of the task definition, which I think is meaningless here.

Hi @SbloodyS @ruanwenjun, thanks for the review — you're right that a task-level row limit shouldn't be an invisible config.

I'd split it into two levels:

Task level: expose a "max query rows" field in the SQL task definition UI, backed by the existing limit in SqlParameters. The task author sets it per task, and it takes precedence over the default.

Deployment level: keep sql.task.query.max.rows as the common.properties fallback used only when a task leaves the field unset. It's essentially an OOM guard for the worker, so it belongs to the operator, which is why the README documents it — the same treatment as the existing sql.task.alert.max.* knobs.

So the override is user-facing in the task UI, while the global default stays an operator-facing knob. I can add the UI field in this PR or as a small follow-up — let me know which you prefer.

[ruanwenjun]

The main purpose of the limit restriction here is to prevent a large amount of data from being loaded into memory and causing the service to run out of memory (OOM). Could we print the results directly to the log after the query is executed, rather than caching all the data and outputting it in JSON format?

[njnu-seafish]

The main purpose of the limit restriction here is to prevent a large amount of data from being loaded into memory and causing the service to run out of memory (OOM). Could we print the results directly to the log after the query is executed, rather than caching all the data and outputting it in JSON format?

Hi @ruanwenjun, thanks for the comment!

Agreed — the row limit here is essentially an OOM guard rather than a functional setting.

One thing to note before switching to log-only output: the returned JSON is not used just for display, it has two existing consumers:

OUT parameters — dealOutParam(result) parses this JSON to fill the varPool, so downstream tasks can reference the values via ${prop}; for LIST-typed OUT parameters the whole column is passed down.
sendEmail — when enabled, the email content is built from the full result (bounded by the sql.task.alert.max.* properties).
In those cases the full result has to stay in memory anyway, so printing each row to the log and discarding it would break the downstream-parameter and alert contracts.

That said, you're right that in the common case — no OUT parameters, no sendEmail — today we are caching all the data and outputting it in JSON format for nothing: nothing consumes it except a debug log. A middle ground could be to buffer the result only when it actually has a consumer (OUT parameters or sendEmail), and iterate the ResultSet forward without buffering otherwise, printing at most displayRows rows to the log as now. That would remove the memory spike the row limit is meant to guard, while the limit still protects the cases that must buffer. Meanwhile the configurable default stays useful for exactly those tasks.

If you think this is worth pursuing, I'd suggest doing it as a follow-up issue/PR to keep this one focused on the configurability — but happy to go either way, what do you prefer?

[ruanwenjun]

LGTM

[SbloodyS]

Reject invalid or non-positive query limits when loading the configuration

[Fact] queryMaxRows is loaded through PropertyUtils.getInt(key, QUERY_LIMIT) in SqlTask.java.

[Fact] The underlying property conversion catches parsing exceptions and returns the supplied default. Therefore, a value such as sql.task.query.max.rows=1000x is silently replaced with 10000, even though the administrator explicitly supplied an invalid value.

[Fact] A configured value of 0 is passed to Statement.setMaxRows(0), which means “no limit” according to the JDBC specification. A negative value is also accepted during configuration loading and fails only later when a query prepares its statement.

[Inference] Because this setting is intended to protect workers from excessive result materialization and OOM, silently using a different limit—or accidentally disabling the limit entirely—defeats its operational purpose and makes configuration errors difficult to diagnose.

[Inference] Please validate this property once when it is loaded: distinguish a missing value from an invalid value, require a positive integer, and throw an exception that includes the property name and invalid value. Please also cover malformed, zero, and negative values in the tests.

Additional scope note

[Fact] The PR does not add a SQL-task UI field for the existing task-level limit; the new Javadoc explicitly says that the value is only settable through API taskParams JSON.

[Fact] Issue #18702 specifically requests deployment-time configurability of the hard-coded fallback and does not explicitly require a UI field.

[Inference] The missing UI field is therefore not a correctness defect against the linked issue, but the existing review discussion should be explicitly resolved before merging if UI exposure is still expected in this PR.

Validation boundary

[Fact] This review covers PR head 73cc68c321f3cc6e7177e754972d5c099fda73b4 against base 0ea315529887fbbe636b30455aa344cec712a559.

[Fact] The complete 10-file diff, linked issue, documentation, Helm configuration, unit-test changes, existing review discussion, and title status were inspected.

[njnu-seafish]

Reject invalid or non-positive query limits when loading the configuration

[Fact] queryMaxRows is loaded through PropertyUtils.getInt(key, QUERY_LIMIT) in SqlTask.java.

[Fact] The underlying property conversion catches parsing exceptions and returns the supplied default. Therefore, a value such as sql.task.query.max.rows=1000x is silently replaced with 10000, even though the administrator explicitly supplied an invalid value.

[Fact] A configured value of 0 is passed to Statement.setMaxRows(0), which means “no limit” according to the JDBC specification. A negative value is also accepted during configuration loading and fails only later when a query prepares its statement.

[Inference] Because this setting is intended to protect workers from excessive result materialization and OOM, silently using a different limit—or accidentally disabling the limit entirely—defeats its operational purpose and makes configuration errors difficult to diagnose.

[Inference] Please validate this property once when it is loaded: distinguish a missing value from an invalid value, require a positive integer, and throw an exception that includes the property name and invalid value. Please also cover malformed, zero, and negative values in the tests.

Additional scope note

[Fact] The PR does not add a SQL-task UI field for the existing task-level limit; the new Javadoc explicitly says that the value is only settable through API taskParams JSON.

[Fact] Issue #18702 specifically requests deployment-time configurability of the hard-coded fallback and does not explicitly require a UI field.

[Inference] The missing UI field is therefore not a correctness defect against the linked issue, but the existing review discussion should be explicitly resolved before merging if UI exposure is still expected in this PR.

Validation boundary

[Fact] This review covers PR head 73cc68c321f3cc6e7177e754972d5c099fda73b4 against base 0ea315529887fbbe636b30455aa344cec712a559.

[Fact] The complete 10-file diff, linked issue, documentation, Helm configuration, unit-test changes, existing review discussion, and title status were inspected.

Hi @SbloodyS, thanks for the detailed review.

Agreed on the validation. PropertyUtils.getInt goes through IPropertyDelegate#get(key, transform, defaultValue), which swallows conversion failures, and setMaxRows(0) means unlimited per the JDBC spec — so the silent fallback and zero/negative values were real problems for an OOM guard.

SqlTask now reads the raw value via PropertyUtils.getString and validates it when the task is constructed:

missing value → falls back to the default (10000)
blank, malformed, zero or negative → IllegalArgumentException naming sql.task.query.max.rows and the invalid value
new tests cover missing, malformed, blank, zero and negative values (commit 46ac583)

[SbloodyS]

Preserve the parsing exception instead of catching it as ignored

[Fact] In SqlTask.java, NumberFormatException is caught as ignored, and a new IllegalArgumentException is thrown without retaining the original cause.

[Inference] Although the replacement message identifies the property and invalid value, discarding the parsing exception removes the original failure chain and conflicts with the fail-fast and observability requirements.

[Inference] Please preserve the cause for parsing failures and handle non-positive parsed values separately. For example:

final int rows;
try {
    rows = Integer.parseInt(value.trim());
} catch (NumberFormatException exception) {
    throw new IllegalArgumentException(
            String.format(
                    "Property %s must be a positive integer, but got: %s",
                    Constants.SQL_TASK_QUERY_MAX_ROWS,
                    value),
            exception);
}
if (rows <= 0) {
    throw new IllegalArgumentException(String.format(
            "Property %s must be a positive integer, but got: %s",
            Constants.SQL_TASK_QUERY_MAX_ROWS,
            value));
}
return rows;

[njnu-seafish]

Preserve the parsing exception instead of catching it as ignored

[Fact] In SqlTask.java, NumberFormatException is caught as ignored, and a new IllegalArgumentException is thrown without retaining the original cause.

[Inference] Although the replacement message identifies the property and invalid value, discarding the parsing exception removes the original failure chain and conflicts with the fail-fast and observability requirements.

[Inference] Please preserve the cause for parsing failures and handle non-positive parsed values separately. For example:

final int rows;
try {
    rows = Integer.parseInt(value.trim());
} catch (NumberFormatException exception) {
    throw new IllegalArgumentException(
            String.format(
                    "Property %s must be a positive integer, but got: %s",
                    Constants.SQL_TASK_QUERY_MAX_ROWS,
                    value),
            exception);
}
if (rows <= 0) {
    throw new IllegalArgumentException(String.format(
            "Property %s must be a positive integer, but got: %s",
            Constants.SQL_TASK_QUERY_MAX_ROWS,
            value));
}
return rows;

Hi @SbloodyS, good catch. I've reworked readQueryMaxRows to parse the value first and preserve the NumberFormatException as the cause when parsing fails, and to handle non-positive parsed values in a separate branch. I also added testQueryMaxRowsRejectsMalformedValuePreservingCause asserting the cause is retained.

[SbloodyS]

I didn't see you modify the UI code to expose this parameter to the user. Did you miss the code commit?

[njnu-seafish]

I didn't see you modify the UI code to expose this parameter to the user. Did you miss the code commit?

Hi @SbloodyS, thanks for the heads-up — that commit was indeed missing. I've now exposed the task-level "max query rows" field in the SQL task definition UI (commit c1e6537): it takes precedence when set, and falls back to the sql.task.query.max.rows default when left blank.