qiuyanjun888 commented on issue #17263:
URL:
https://github.com/apache/dolphinscheduler/issues/17263#issuecomment-4766155929
I looked into the current `SqlTask` implementation and the related
issues/PRs (#17124, #17343, #18231). I agree that the current
PreparedStatement-based replacement is a bad fit for some SqlTask scenarios,
especially when `${}` is used in SQL identifiers or DDL statements such as
`create table test_${dd}`. In those cases the placeholder becomes a JDBC
parameter marker, which can produce invalid SQL semantics depending on the
driver.
## Proposed direction
Change SqlTask parameter handling from JDBC bind parameters to SqlTask-local
SQL rendering before execution.
### 1. Keep the change local to SqlTask
The first implementation should avoid changing global `ParameterUtils`
behavior or datasource plugins. It should be limited to the SQL task execution
path:
- render `${}` / `!{}` into the SQL string before execution;
- execute the rendered SQL with `Statement` rather than `PreparedStatement`;
- preserve existing SQL splitting, pre/main/post execution order, query
timeout, max rows, cancel support, result processing, alert attachment, output
params and varPool behavior.
This keeps the first PR smaller than the old WIP #17343 and reduces conflict
with other task plugins.
### 2. Define rendering rules explicitly
The main compatibility question is how `${}` should behave after removing
PreparedStatement binding.
Possible rule set:
- numeric/boolean parameters render as unquoted literals;
- string/date/time/timestamp parameters render as escaped SQL string
literals, at least escaping single quotes (`'` -> `''`);
- list parameters render as comma-separated literals for `IN (...)` use
cases;
- `!{}` keeps raw textual replacement semantics for cases where users
intentionally need raw SQL fragments;
- identifier/DDL cases such as `test_${dd}` should work when the rendered
value is intended as an identifier fragment.
The difficult part is that `${name}` can be used either as a value literal
or as part of an identifier. We should agree on this before implementation,
because existing SQL like `where name=${name}` worked with PreparedStatement
even without quotes.
### 3. Suggested implementation shape
- Add a small SqlTask-local renderer with unit tests.
- Adapt `SqlBinds` or introduce a new internal value object so the name no
longer implies JDBC binding.
- Replace `prepareStatementAndBind()` with a `createStatement()` helper that
still sets query timeout / max rows and stores `sessionStatement` for
cancellation.
- Update query/update execution to call
`Statement.executeQuery(renderedSql)` / `Statement.executeUpdate(renderedSql)`.
- Coordinate with #18231 so regex/raw replacement escaping is not solved
twice in conflicting ways.
### 4. Test plan
A first PR should include tests for:
- `create table test_${dd}` rendering to `create table test_20250411`;
- string literal escaping;
- list parameter rendering for `in (${ids})`;
- `!{}` replacement with `$` or `\` characters;
- schedule-time replacement still working;
- existing result-processing and out-param behavior still working.
## Questions for maintainers
1. Should `${}` become a value-literal renderer with automatic quoting for
strings, or should it be raw textual replacement according to the DSIP wording?
2. Should `!{}` keep its current raw replacement semantics, or should it be
deprecated/merged with `${}`?
3. Is the preferred first implementation a small SqlTask-local change, or
should it revive the larger execution-engine refactor attempted in #17343?
4. Should the first implementation use a generic escaping rule across
datasources, or should datasource-specific rendering be introduced from the
beginning?
5. How should this work be sequenced with the active parameter replacement
PR #18231?
If this direction is acceptable, I can prepare a focused PR starting with
renderer tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
