njnu-seafish commented on PR #18705: URL: https://github.com/apache/dolphinscheduler/pull/18705#issuecomment-6162017119
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 (52e187d832) 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. -- 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]
