aminghadersohi commented on PR #42244: URL: https://github.com/apache/superset/pull/42244#issuecomment-5041645865
Thanks for the thorough review — went through each point: **🔴 High — `execute_sql` row list serialized twice.** Verified this empirically before touching anything: `_linked_statement_row_lists` (added in the prior `ea1e5e3` fix, for the wrapper-overhead bug) already trims `statements[*].data.rows` in lockstep with the top-level `rows` field during bisection, so the "headline fix doesn't fire" concern doesn't hold — a simulated oversized single-`SELECT` `execute_sql` response truncates correctly and the final wrapped size comes in under the limit. What *was* real: `statements[*].row_count` (a separate field copied at response-build time) wasn't updated when `data.rows` got trimmed — so a truncated response could show `data.rows` with 8 entries while `row_count` still said 2000. Fixed in `00c5e79`: renamed `_linked_statement_row_lists` → `_linked_statement_entries` to track the statement dicts themselves (not just their row lists), and `row_count` is now set alongside `data.rows` on every bisection step. Added `test_execute_sql_statements_row_count_stays_in_sync`. **🟡 Medium — missing truncation markers on the `row_field is None` fallback.** Already fixed in `0d9bc32` (pushed before this review) — `_try_truncate_data_query_response` now sets `_response_truncated`/`_truncation_notes` explicitly, mirroring the info-tool path, so this covers the fallback branch too (the row/CSV path already set them internally). Added `test_falls_back_to_generic_truncation_when_no_row_field` to cover that branch directly. **🟡 Medium — test gaps.** Added: - `test_data_query_truncation_preserves_total_rows` — `row_count=200`, `total_rows=5000`, asserts `total_rows` survives while `row_count` is overwritten to the truncated count. - `test_falls_back_to_generic_truncation_when_no_row_field` — exercises the `truncate_oversized_response` fallback directly. - `test_get_chart_data_excel_export_raises_tool_error` — oversized excel export through `on_call_tool`, asserts `pytest.raises(ToolError)`. **🟢 Low — CSV truncation cuts mid-row.** Checked this one and don't think it's live: the function actually responsible for `csv_data` (`_bisect_csv_row_limit`) already cuts on CSV record boundaries via the `csv` module (respects quoted fields/embedded newlines), not an arbitrary character offset — there's an existing regression test for exactly this (`test_csv_truncation_keeps_header_and_whole_records`) with a quoted-comma-and-newline field that would fail on a naive character cut. `_bisect_string_length` doesn't exist in this file; I couldn't find the referenced line, so I suspect this was a mix-up with the generic `_truncate_strings` phase (which *does* do a raw character cut, but only runs when there's no recognised row/data field at all — and every current data-query tool that emits `csv_data` also emits `data=[]`, which routes it through the record-boundary-safe path instead). Let me know if you had a specific reachable path in mind that I'm missing. **🟢 Low — broad `except Exception`.** Kept the broad catch (it's intentionally fail-safe — narrowing it wouldn't change behavior since any exception already falls through to the hard `ToolError`), but switched `logger.warning` → `logger.exception` in `00c5e79` so a genuine bug gets a full traceback instead of blending in with routine size-limit enforcement. Rebased onto master (no conflicts) and pushed. Full commit list: `72df1f7` (original fix) → `984a491` (wrapper-overhead re-check) → `0d9bc32` (fallback truncation markers) → `00c5e79` (this round). -- 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]
