fix(sqllab): preserve exact decimals in results and exports

[aminghadersohi]

SUMMARY

SQL Lab sends Decimal values as JSON numbers. Even when the JSON contains all digits, JavaScript rounds them; reading the JSON result cache also loses precision before CSV export.

Convert decimals to exact fixed-point strings (format(value, "f"), so DECIMAL(38,18) zeros stay 0.000000000000000000 rather than 0E-18) at the SQL Lab record boundary, shared by synchronous results, JSON cache writes and Arrow-cache reads. Columns PyArrow cannot type as decimal (Decimal NaN/Infinity, or Decimals mixed with floats) are stringified the same way, with NaN and Infinity returned as null. Keep the DataFrame and shared chart JSON serializer numeric. Sort decimal strings by sign, decimal order and significand rather than parseFloat, including values differing only in their 38th digit. The grid comparator orders by type first (numbers, including exact decimal strings and infinities, then text, then NaN, then nulls), so mixed columns sort the same regardless of input order. Two plain numbers are compared directly, and parsed sort keys are cached per value. API behavior, including strings for numeric columns in sqlLab.onDidQuerySuccess extension listeners, is documented in UPDATING and the user guide.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before: 12345678901234567890.123456789012345678 is a JSON number and rounds in the browser. After: the response contains the exact quoted value; the grid displays it and sorts it numerically. A component test clicks the column header and checks both directions.

TESTING INSTRUCTIONS

• Backend dataframe tests: 39 pass after merging master, including high-scale zero and small-negative cases and a CSV export check that -0.000000100000000000 is not formula-escaped.

• FilterableTable/component/comparator Jest tests: 67 pass locally. Six new comparator tests (type-first ordering, the [9, '5x', 10] permutations and a brute-force transitivity/antisymmetry check over mixed values) fail with the previous comparator. The parser-equivalence check agrees with the reference regex on all 111,111 enumerated inputs.

• Result-set tests: 24 pass, including [Decimal('Infinity'), Decimal('-0.00000010')], Decimal NaN and a Decimal/float mix, with a CSV export check. The three new cases fail before the stringify_values change.

• Sort benchmark (Node 22, 100k rows, median of 5 runs):

  Column	master	before this revision	after
  plain numbers	30 ms	925 ms	32 ms
  decimal strings	500 ms	1294 ms	226 ms
  text	102 ms	467 ms	140 ms

• Live PostgreSQL 17 and Databricks: exact DECIMAL(38,18) through SQL Lab HTTP, sync execution, JSON and Arrow caches, CSV exports; live chart SUM stays numeric. The upstream implementation was run separately without a serialization carry.

• Pre-commit checks pass on the changed files, including the frontend type check. No dependency or lockfile changes.

To reproduce manually, execute SELECT CAST('12345678901234567890.123456789012345678' AS DECIMAL(38,18)) AS exact in SQL Lab, inspect the JSON response, sort nearby decimal values and export CSV. Re-run old cached queries after upgrading workers.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration
• Introduces new feature or API: decimal response values become strings
• Removes existing feature or API

[netlify]

✅ Deploy Preview for superset-docs-preview ready!

Name	Link
🔨 Latest commit	8f54a98
🔍 Latest deploy log	https://app.netlify.com/projects/superset-docs-preview/deploys/6abd7a74219dcc00097819c1
😎 Deploy Preview	https://deploy-preview-44739--superset-docs-preview.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
🤖 Make changes	Run an agent on this branch

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.67%. Comparing base (16271bb) to head (e1fb34c).
⚠️ Report is 23 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #44739      +/-   ##
==========================================
+ Coverage   81.58%   81.67%   +0.08%     
==========================================
  Files        2977     2978       +1     
  Lines      180822   181348     +526     
  Branches    41849    41921      +72     
==========================================
+ Hits       147531   148117     +586     
+ Misses      30568    30508      -60     
  Partials     2723     2723              

Flag	Coverage Δ
hive	36.67% <33.33%> (+0.01%)	⬆️
javascript	77.08% <100.00%> (+0.02%)	⬆️
mysql	55.85% <48.71%> (-0.06%)	⬇️
postgres	55.85% <48.71%> (-0.07%)	⬇️
presto	38.59% <35.89%> (+<0.01%)	⬆️
python	85.86% <100.00%> (+0.13%)	⬆️
sqlite	55.58% <48.71%> (-0.07%)	⬇️
unit	78.70% <97.43%> (+0.18%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bito-code-review]

Code Review Agent Run #f49a1c

Actionable Suggestions - 3

• superset-frontend/src/components/FilterableTable/sortResults.ts - 1

  • CWE-20: Comparator contract violations · Line 59-63
• tests/unit_tests/dataframe_test.py - 2

  • Missing test docstrings · Line 381-381
  • Local shadows stdlib module name · Line 382-382

Additional Suggestions - 1

• superset-frontend/src/components/FilterableTable/sortResults.ts - 1

  • Unsafe regex with potential ReDoS · Line 23-23
    The regular expression on line 23 (`DECIMAL`) is flagged as unsafe (CWE-1333) due to nested quantifiers that may cause catastrophic backtracking. A similar issue exists on line 58. Consider simplifying the pattern to avoid nested quantifiers while preserving the same matching behavior.

Review Details

• Files reviewed - 7 · Commit Range: 0a36b0d..0a36b0d

  • docs/docs/using-superset/number-formatting.mdx
  • superset-frontend/src/components/FilterableTable/exactDecimals.test.tsx
  • superset-frontend/src/components/FilterableTable/index.tsx
  • superset-frontend/src/components/FilterableTable/sortResults.test.ts
  • superset-frontend/src/components/FilterableTable/sortResults.ts
  • superset/dataframe.py
  • tests/unit_tests/dataframe_test.py
• Files skipped - 1

  • UPDATING.md - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Addressed bito's additional suggestion (unsafe regex with nested quantifiers, sortResults.ts line 23 and the similar one on line 58) in 394a03d. Both patterns are replaced with hand-split sign/fraction/exponent parsing, so each remaining regex is a single flat run of digits and matching is linear. A new test checks the parsers against the old patterns on all 111,111 strings of up to five tokens (identical results) and runs 200k-character near-miss inputs under a time bound.

Also merged master in (cd7c69f). Master now sends chart JSON through df_to_records, so decimal quoting is opt-out there: chart records keep Decimal numbers (convert_decimals=False in query_context_processor, with a test), and SQL Lab keeps exact strings.

[bito-code-review]

Code Review Agent Run #f7252e

Actionable Suggestions - 0

Additional Suggestions - 4

• superset-frontend/src/components/FilterableTable/sortResults.test.ts - 4

  • Redundant checked counter · Line 88-103
    `checked` is incremented per string (lines 91, 99) and only read by `expect(checked).toBeGreaterThan(100000)` (line 102). The count is fully determined by the alphabet (10 tokens) and `maxLength` 5: 111111 strings — verified by running the same enumeration on Node 20.12.1. The threshold also passes even if `strings` silently returns a truncated enumeration (e.g. after a swallowed spread failure), so it adds a mutable counter without real protection.
  • NaN branch untested · Line 89-92
    The alphabet includes the multi-char tokens `'NaN'` and `'Infinity'` (line 89), but with `maxLength: 5` (line 92) neither can ever appear in an enumerated string (`'NaN'` needs 3 chars only as a whole token — flatMap concatenation can produce it, but `'Infinity'` at 8 chars cannot; and the adversarial test at lines 106-111 never feeds `NaN`-prefixed inputs). The oracle equivalence for the `NaN` branch of `isNumericText` (sortResults.ts line 65) is therefore never actually exercised by either new test — verified by executing the enumeration: 111111 strings, zero containing 'Infinity'.
  • Flaky wall-clock assertion · Line 112-117
    The wall-clock bound `expect(Date.now() - started).toBeLessThan(1000)` (line 117) is a flake risk: measured parse time for the four 200k-char inputs is ~3ms on this machine (Node 20.12.1), but shared CI runners can transiently stall >1s between `Date.now()` samples, failing the test without any parser regression. The repo's jest config sets `testTimeout: 20000`, so a looser bound is consistent with existing practice.
  • Spread RangeError risk · Line 78-86
    `strings()` builds the enumeration with `all.push(...level)`. Spread arguments are subject to the engine's max call-argument limit: on Node 20 a spread of ~1M elements throws `RangeError: Maximum call stack size exceeded`. The limit is 5 today, but raising `maxLength` (e.g. to 7 for the 8-char `Infinity` token) makes the test crash with a RangeError instead of reporting mismatches. Copy per element or use `concat`.

Review Details

• Files reviewed - 2 · Commit Range: 0a36b0d..394a03d

  • superset-frontend/src/components/FilterableTable/sortResults.test.ts
  • superset-frontend/src/components/FilterableTable/sortResults.ts
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[EnxDev]

Went through the backend boundary and the comparator. The chart path staying numeric and the Arrow read path getting the same conversion both check out.

One thing to fix before merge, inline on _convert_decimals: str(Decimal) switches to scientific notation for high-scale values, so zeros in a DECIMAL(38,18) column show up as 0E-18. Plus a small nit on a test comment.

[aminghadersohi]

Code Review Agent Run #f7252e: redundant checked counter, NaN branch untested, flaky wall-clock assertion, spread RangeError risk

Addressed in 730a8ed: the counter and the Date.now() bound are gone. The test asserts the enumeration has exactly 111,111 entries, and strings() pushes per element rather than spreading. On the NaN point: maxLength counts tokens, not characters, so NaN, Infinity and -Infinity are each one- or two-token strings. The test now asserts they are in the enumeration, so the NaN/Infinity branches of isNumericText are compared against the reference regex. Master merged in 2412b52.

[fitzee]

Verdict: Needs changes (1 only). The backend is correct; items 2–4 are nits.

I verified fe32227 against master locally: SupersetResultSet → _serialize_and_expand_data (JSON and msgpack) → _deserialize_results_payload → CSV export.

• Output is exact for 38-digit decimal128, 60-digit decimal256, 1E-40, 1E+30/-5E+3, DECIMAL(18,18) 0 and -1E-7, Decimal mixed with int, and NULLs.
• Chart JSON is still numeric.
• Payloads written by old and new workers are readable by both sides, so rolling deploys are fine.
• Targeted and surrounding unit tests pass (6k+).

1. Sorting numeric columns got ~40x slower (FilterableTable/sortResults.ts:72-86). Every comparison, including number-vs-number, runs String(), regexes, BigInt and padEnd.

   • Node 22, 100k rows: plain numbers 37ms → 1726ms, decimal strings 459 → 1428ms, text 173 → 581ms.
   • At the default DISPLAY_MAX_ROW (10k), numbers go 3 → 125ms.

   Add a fast path at the top, and consider memoizing decimalParts per value:

   if (typeof valueA === 'number' && typeof valueB === 'number' &&
       !Number.isNaN(valueA) && !Number.isNaN(valueB)) {
     return valueA === valueB ? 0 : valueA < valueB ? -1 : 1;
   }

2. The comparator still isn't transitive (sortResults.ts:101-107). Numeric pairs compare numerically, but number-vs-text pairs compare lexicographically.

   • sortResults('9','10'), ('10','5x') and ('5x','9') all return -1.
   • So [9,'5x',10], [10,9,'5x'] and ['5x',10,9] sort to three different orders.
   • The "sorts a mixed text and number column transitively" test only passes because of the values it uses.

   Order by type first (numeric < text < NaN), then compare within each type. This was already broken on master, but the comment says it's fixed.

3. Scientific notation is still possible (result_set.py:94). When pyarrow can't infer a decimal type, the column is stringified with str(Decimal). That happens when the column contains Decimal NaN or Infinity (both valid Postgres numeric values), or Decimal mixed with float.

   • [Decimal('Infinity'), Decimal('-0.00000010')] becomes "Infinity", "-1.0E-7", and CSV writes '-1.0E-7 (formula-escaped).
   • Master does the same, but it contradicts "never in scientific notation" in number-formatting.mdx.

   Use format(v, "f") for finite Decimals and None for non-finite ones in stringify_values, or soften the doc line.

4. Extension API (src/core/sqlLab/index.ts:246): onDidQuerySucceed result.data now carries strings for numeric columns (for example Postgres SUM(bigint)/AVG/ROUND). An extension that adds these values would concatenate them ("12"+"3" = "123"). Please mention extensions in the UPDATING entry.

[aminghadersohi]

@fitzee thanks for the thorough review. All four points are addressed; master had not moved, so there's no new merge.

1. Sort performance (8d33239). Your number-vs-number fast path is at the top of sortResults, and each value's parsed sort key is memoized in a bounded cache that clears above 250k entries. Node 22, 100k rows, median of 5:

   Column	master	fe32227	8d33239
   plain numbers	30 ms	925 ms	32 ms
   decimal strings	500 ms	1294 ms	226 ms
   text	102 ms	467 ms	140 ms

   New tests cover direct number comparisons (-0/0, infinities, 0.1 + 0.2 vs 0.3). They also check that a number and its exact string, such as 1e21 and '1E+21', compare equal, so the fast path agrees with the parsed path.

2. Transitivity (8d33239). The comparator orders by type first: numbers (exact decimal strings and ±Infinity included), then text, then NaN, then nulls. It then compares within each type. [9,'5x',10], [10,9,'5x'], ['5x',10,9] and their string versions all sort to 9, 10, '5x'. A brute-force test checks reflexivity, antisymmetry and transitivity over every triple of 23 mixed values. The misleading comment is gone, and so is isNumericText, which only served the old text fallback. Six of the new tests fail against the previous comparator. FilterableTable Jest: 67 pass locally.

3. Scientific notation fallback (0241530). stringify_values uses format(v, "f") for finite Decimals and None for NaN/Infinity. Tests cover [Decimal('Infinity'), Decimal('-0.00000010')] → [None, '-0.00000010'], Decimal NaN/-Infinity/1E+3, and Decimal mixed with float (-0.00000010, 1.5, 0E-18). Each test also asserts the CSV has no apostrophe escape and no E± notation. All three fail before the change. This keeps the "never in scientific notation" line in the docs accurate.

4. Extension API (1b68a7b). The UPDATING entry says the data rows given to sqlLab.onDidQuerySuccess listeners carry strings for numeric columns, and that + concatenates them. The API is named onDidQuerySuccess in @apache-superset/core. The entry also notes fixed-point notation and null for non-finite decimals.

The PR description is updated with the new behaviour and the benchmark.

[bito-code-review]

Code Review Agent Run #eb6d92

Actionable Suggestions - 1

• superset-frontend/src/components/FilterableTable/sortResults.test.ts - 1

  • test.each row spread bug · Line 73-83

Review Details

• Files reviewed - 7 · Commit Range: 394a03d..1b68a7b

  • docs/docs/using-superset/number-formatting.mdx
  • superset-frontend/src/components/FilterableTable/sortResults.test.ts
  • superset/dataframe.py
  • tests/unit_tests/dataframe_test.py
  • superset-frontend/src/components/FilterableTable/sortResults.ts
  • superset/result_set.py
  • tests/unit_tests/result_set_test.py
• Files skipped - 1

  • UPDATING.md - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[EnxDev]

Second pass. This supersedes my earlier review.

Both earlier points are fixed in 730a8ed. _convert_decimals uses format(value, "f"), the high-scale zero and small negative cases are in the parametrize, and there's a CSV assertion. The reference regex rename landed too. fitzee's four points also check out in the diff: the number fast path, the type-ranked comparator with the total-order test, fixed-point stringify_values, and the extension note. The event name onDidQuerySuccess matches @apache-superset/core.

Two new notes inline. The module-level sort key cache keeps cell values alive after the grid is gone. And the stringify_values change reaches chart queries, which the UPDATING entry says are unchanged. Also, bito's test.each note on sortResults.test.ts:83 is a false positive: each row is [[...]], so column gets the whole array.

[EnxDev]

Third pass, superseding my previous one.

Both notes from last time are fixed at d161f30. The sort key cache is a WeakMap keyed by the rows array, and data is a stable Redux reference in ResultSet, so columns only rebuild on a new result. The UPDATING entry and the guide cover the stringify_values fallback for charts, with tests through Database.load_into_dataframe. I also checked a Postgres-style numeric[] column locally through SupersetResultSet and df_to_records, and the nested decimals come out exact too. CI is green.

Looks good to me. Left two small notes inline, on the convert_decimals default and one gap in the UPDATING entry, plus a test nit you can skip.

[bito-code-review]

Code Review Agent Run #ce5235

Actionable Suggestions - 1

• superset-frontend/src/components/FilterableTable/index.tsx - 1

  • Unstable comparator identity · Line 71-72

Additional Suggestions - 3

• tests/unit_tests/result_set_test.py - 2

  • Duplicate test coverage · Line 691-703
    This parametrize block duplicates `test_stringified_decimals_use_fixed_point` (lines 650-688): the `[Decimal("-0.00000010"), 1.5, Decimal("0E-18")]` case is identical and the non-finite cases overlap. Both exercise the same `SupersetResultSet` stringify path. Consolidate or use distinct cases to avoid divergent maintenance.
  • Test DB side effect · Line 714-714
    `Database.load_into_dataframe` is wrapped by `@event_logger.log_this`, so this test triggers a real `DBEventLogger` DB write (`db.session.commit()`) on every parametrized run. That couples the test to the event-logging stack and pollutes the test DB. Mock `event_logger.log` or call `SupersetResultSet(...).to_pandas_df()` directly.
• superset-frontend/src/components/FilterableTable/sortResults.test.ts - 1

  • Vacuous cache-pollution guard · Line 237-237
    `sortResults('1.25', '2.50')` without `rows` returns early in `sortKey` (sortResults.ts:129) and never touches `sortKeyCaches`, so `getCachedSortKey('1.25', rows)` here cannot fail — the assertion implies the rows-less path could pollute the cache when it structurally cannot. Drop it; line 236 and the `test.each` at lines 21-50 already cover the rows-less overload.

Review Details

• Files reviewed - 11 · Commit Range: 1b68a7b..48bf663

  • superset-frontend/src/components/FilterableTable/index.tsx
  • superset-frontend/src/components/FilterableTable/sortResults.test.ts
  • superset-frontend/src/components/FilterableTable/sortResults.ts
  • docs/docs/using-superset/number-formatting.mdx
  • tests/unit_tests/result_set_test.py
  • superset/common/query_context_processor.py
  • superset/dataframe.py
  • superset/sql/execution/celery_task.py
  • superset/sql_lab.py
  • superset/views/utils.py
  • tests/unit_tests/dataframe_test.py
• Files skipped - 1

  • UPDATING.md - Reason: Filter setting
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful
  • Eslint (Linter) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[bito-code-review]

Code Review Agent Run #7124f5

Actionable Suggestions - 0

Review Details

• Files reviewed - 2 · Commit Range: 48bf663..de3c51c

  • superset-frontend/src/components/FilterableTable/exactDecimals.test.tsx
  • superset-frontend/src/components/FilterableTable/index.tsx
• Files skipped - 0
• Tools

  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Fixed in 8f54a98: the SQL Lab CSV export path without a results backend and the streaming CSV export path now format finite Decimal values as exact fixed-point text, matching the no-scientific-notation promise. Added regression coverage for zero, negative and large exponents, trailing zeros, custom decimal separators, and unchanged non-finite values. Both touched unit-test files pass (35 tests), and pre-commit passes for all five changed files.

[EnxDev]

Reviewed 8f54a98140, including the CSV export follow-up pushed during this pass. No new actionable findings. This supersedes my previous review.

The earlier notes are addressed: convert_decimals defaults to False, all three SQL Lab callers opt in, the result-table extension contract is documented, and the cache test checks key reuse without spying on BigInt. The shared comparator retains the correct rows-array dependency, with rerender coverage. The latest export change also covers re-executed queries and streaming CSV, including custom decimal separators.

[bito-code-review]

Code Review Agent Run #1a674f

Actionable Suggestions - 2

• superset/commands/sql_lab/export.py - 2

  • Formatting logic in run() · Line 144-148
  • Per-cell .at loop slow · Line 144-148

Additional Suggestions - 2

• superset/commands/streaming_export/base.py - 1

  • Decimal handled twice · Line 195-199
    New `Decimal` branch (195-199) overlaps the `(float, Decimal, Real)` tuple at line 203: finite Decimals are caught here, so `Decimal` in that tuple now only reaches non-finite values (NaN/Infinity), which have no `.` to replace. Consider dropping `Decimal` from the tuple to avoid two divergent Decimal paths (fixed vs scientific formatting).
• tests/unit_tests/commands/sql_lab/streaming_export_command_test.py - 1

  • Missing test local type hints · Line 923-923
    New test locals (`result`, `command`, `buffer`, `writer`, `chunks`, `values`, `formatted`) lack the explicit type annotations BITO.md adaptive rules 12787/13153 mandate for test files, e.g. `result: MagicMock = MagicMock()`. Params and return types are annotated; extend the same coverage to locals so mypy and reviewers see intended types. Applies in both new tests (lines 922-933 and 943-954).

Review Details

• Files reviewed - 5 · Commit Range: de3c51c..8f54a98

  • docs/docs/using-superset/number-formatting.mdx
  • superset/commands/sql_lab/export.py
  • superset/commands/streaming_export/base.py
  • tests/unit_tests/commands/sql_lab/export_test.py
  • tests/unit_tests/commands/sql_lab/streaming_export_command_test.py
• Files skipped - 0
• Tools

  • MyPy (Static Code Analysis) - ✔︎ Successful
  • Astral Ruff (Static Code Analysis) - ✔︎ Successful
  • Whispers (Secret Scanner) - ✔︎ Successful
  • Detect-secrets (Secret Scanner) - ✔︎ Successful

---

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

• /review - Manually triggers an incremental AI Review.

• /review full - Manually triggers a full AI Review.

• /pause - Pauses automatic reviews on this pull request.

• /resume - Resumes automatic reviews.

• /resolve - Marks all Bito-posted review comments as resolved.

• /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

• Customize agent settings
• Review rules
• General documentation
• FAQ

AI Code Review powered by

[aminghadersohi]

Fixed both body suggestions in e1fb34c: consolidated Decimal handling and added test-local annotations. NaN/Infinity and custom decimal separators are covered by regression tests. All 55 targeted tests and changed-file pre-commit checks pass.

[rusackas]

Thanks for chasing down fitzee's notes, the number-vs-number fast path and cached sort keys in sortResults.ts take care of the perf regression, and result_set.py's fallback formatting handles the scientific-notation case now too. Also checked commands/sql_lab/export.py's no-cache branch myself since it looked like it could've skipped the same fix, it's routed through _format_decimal_columns now as well. LGTM!

[rusackas]

Thanks for chasing down fitzee's notes, the number-vs-number fast path and cached sort keys in sortResults.ts take care of the perf regression, and result_set.py's fallback formatting handles the scientific-notation case now too. Also checked commands/sql_lab/export.py's no-cache branch myself since it looked like it could've skipped the same fix, it's routed through _format_decimal_columns now as well. LGTM!

[bito-code-review]

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it