ramiroaquinoromero commented on PR #37112: URL: https://github.com/apache/superset/pull/37112#issuecomment-3803247314
Good morning @msyavuz related to this comment. https://github.com/apache/superset/pull/37112#discussion_r2704009010 This is a good point, but both approaches are functionally equivalent for preventing false warnings when no limit is set: Your PR: rowLimit = Number(formData.row_limit || -1) → sqlRowCount === -1 My change: rowLimit = Number(formData.row_limit ?? 0) → rowLimit > 0 check prevents warning No regression occurs because the rowLimit > 0 guard serves the same purpose as your -1 sentinel value. Additional improvement: I changed the comparison from === to >= because the warning should show when the limit is reached or exceeded, not just when exactly equal. For example, if a query returns 101 rows with a limit of 100, we should still show the warning. If you prefer to keep the -1 pattern for consistency with your original implementation, I can update it to: `const rowLimit = Number(formData.row_limit || -1);` Please let me know I can update this. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
