Skip to content

fix(chart-controls): type post-processing columns as a string-to-string mapping - #43589

Merged
rusackas merged 2 commits into
apache:masterfrom
SEPURI-SAI-KRISHNA:fix/post-processing-columns-mapping
Sep 22, 2026
Merged

rusackas merged 2 commits into
apache:masterfrom
SEPURI-SAI-KRISHNA:fix/post-processing-columns-mapping

Conversation

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor

SUMMARY

PostProcessingRolling, PostProcessingCum and PostProcessingDiff declare options.columns as string[], but all three operations take a string-to-string mapping. This corrects the exported types to Record<string, string>.

The mapping is what the runtime has always required. Each operation takes columns: dict[str, str] and indexes the frame by its keys:

operation signature use
rolling() columns: dict[str, str] df.loc[:, columns.keys()]
cum() columns: dict[str, str] df[columns.keys()]
diff() columns: dict[str, str] df[columns.keys()]

An array reaching any of them calls list.keys() and raises AttributeError.

It is also what the frontend already emits. rollingWindowOperator builds a mapping and sends it for both operations it produces:

const columnsMap = Object.fromEntries(columns.map(col => [col, col]));

and the existing tests pin that object shape, rollingWindowOperator.test.ts and plugin-chart-echarts's MixedTimeseries/buildQuery.test.ts both assert columns: { 'count(*)': 'count(*)', ... }. The only string[] in the tree was the fixture block in PostProcessing.test.ts, which used arrays because the type asked for them; those are updated here.

Why this never broke the build. columns.map(col => [col, col]) widens each pair to (string | undefined)[] instead of a tuple, so overload resolution picks the untyped Object.fromEntries:

fromEntries<T = any>(entries: Iterable<readonly [PropertyKey, T]>): { [k: string]: T };
fromEntries(entries: Iterable<readonly any[]>): any;   // <- selected

columnsMap is therefore any, which assigns to string[] without complaint. Annotating the tuple explicitly surfaces the underlying conflict:

error TS2322: Type 'string[]' is not assignable to type 'Record<string, string>'.
  Index signature for type 'string' is missing in type 'string[]'.

That is the error the corrected types now raise if an array is reintroduced.

Follow-up to the discussion on #43225, which restored rolling.columns to the published OpenAPI spec and made the divergence visible to typed clients. That PR is Python-only; this is the frontend half.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Not applicable, type declarations only.

TESTING INSTRUCTIONS

No runtime behaviour changes: the emitted payloads are unchanged, and the isPostProcessing* guards only inspect .operation, so their results are unaffected.

cd superset-frontend
npm run type
npm run test -- packages/superset-ui-core/test/query/types/PostProcessing.test.ts
npm run test -- packages/superset-ui-chart-controls/test/operators/rollingWindowOperator.test.ts

To confirm the corrected types actually constrain, change any columns value in PostProcessing.test.ts back to an array and re-run npm run type; it fails with the TS2322 above.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot Bot added the change:frontend Requires changing the frontend label Aug 27, 2026
@bito-code-review

bito-code-review Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #427a8f

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 7bffa07..7bffa07
    • superset-frontend/packages/superset-ui-core/src/query/types/PostProcessing.ts
    • superset-frontend/packages/superset-ui-core/test/query/types/PostProcessing.test.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

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.20%. Comparing base (c6d0890) to head (9de0ae2).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #43589      +/-   ##
==========================================
+ Coverage   65.92%   66.20%   +0.27%     
==========================================
  Files        2945     2946       +1     
  Lines      176858   176959     +101     
  Branches    40973    41022      +49     
==========================================
+ Hits       116600   117153     +553     
+ Misses      57754    57310     -444     
+ Partials     2504     2496       -8     
Flag Coverage Δ
javascript 76.26% <ø> (+0.04%) ⬆️

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor Author

Note for whoever picks this up: the one red check (lint-frontend) is an infrastructure flake, not a finding against this diff.

The run reports 1863 warnings and 94 errors, and none of the 94 are in either file this PR touches. The tell is the duration, this job finished in 1m14s, where passing runs of the same job take 3m20s–3m35s. npm i is short-circuiting, so oxlint can't resolve imports and rules like import(no-named-as-default) fire across the tree. The same 1863/94 numbers appear verbatim on unrelated PRs, and the warning count varies between runs of identical code.

Running oxlint directly against both changed files reports zero findings. The other 67 checks pass. I don't have permission to re-run the job myself, so I'll refresh it with a rebase if that's easier than a maintainer re-run.

@rusackas rusackas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix — matches what rollingWindowOperator.ts already builds and what the backend's dict[str, str] signature for diff/cum/rolling expects. The lint-frontend failure looks unrelated to this diff (import-resolution errors scattered across the whole tree, nothing in the two touched files); I'll see if I can get that working.

@rusackas
rusackas force-pushed the fix/post-processing-columns-mapping branch from 7bffa07 to 567ab28 Compare September 7, 2026 17:14
@bito-code-review

bito-code-review Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

Code Review Agent Run #be61eb

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 567ab28..567ab28
    • superset-frontend/packages/superset-ui-core/src/query/types/PostProcessing.ts
    • superset-frontend/packages/superset-ui-core/test/query/types/PostProcessing.test.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

AI Code Review powered by Bito Logo

@SEPURI-SAI-KRISHNA

Copy link
Copy Markdown
Contributor Author

Friendly ping, this has been approved since Sept 8. The only red checks are the Netlify docs-preview ones, which aren't required and are unrelated to this change (it touches superset-frontend types only). Anything else needed from me before it can go in?

@rusackas

Copy link
Copy Markdown
Member

We'll get it merged, but hopefully the docs CI checks will clear themselves now :D Thanks for your persistence/patience :)

@rusackas rusackas added the merge-if-green If approved and tests are green, please go ahead and merge it for me label Sep 22, 2026
@rusackas
rusackas merged commit 2b5a929 into apache:master Sep 22, 2026
87 checks passed
@bito-code-review

Copy link
Copy Markdown
Contributor

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

villebro pushed a commit that referenced this pull request Sep 22, 2026
…ng mapping (#43589)

Co-authored-by: Evan Rusackas <evan@preset.io>
(cherry picked from commit 2b5a929)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend merge-if-green If approved and tests are green, please go ahead and merge it for me packages size/S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants