You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "msyavuz (via GitHub)" <gi...@apache.org> on 2025/10/22 08:03:34 UTC
[PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
msyavuz opened a new pull request, #35790:
URL: https://github.com/apache/superset/pull/35790
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35790:
URL: https://github.com/apache/superset/pull/35790#issuecomment-3450804380
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/18839343617) for [108099c](https://github.com/apache/superset/commit/108099c)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35790:
URL: https://github.com/apache/superset/pull/35790#issuecomment-3430946482
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35790:
URL: https://github.com/apache/superset/pull/35790#discussion_r2450831151
##########
superset-frontend/src/components/Datasource/components/DatasourceEditor/DatasourceEditor.jsx:
##########
@@ -1888,6 +1891,55 @@ class DatasourceEditor extends PureComponent {
);
}
+ componentDidUpdate(prevProps) {
+ // Preserve calculated columns order when props change to prevent jumping
+ if (this.props.datasource !== prevProps.datasource) {
+ const newCalculatedColumns = this.props.datasource.columns.filter(col => !!col.expression);
+ const currentCalculatedColumns = this.state.calculatedColumns;
+
+ // Check if this is just an update to existing calculated columns (not addition/removal)
+ if (newCalculatedColumns.length === currentCalculatedColumns.length) {
+ // Create a map of current calculated columns by their identifier
+ const currentMap = new Map();
+ currentCalculatedColumns.forEach((col, index) => {
+ const id = col.id || col.column_name;
+ currentMap.set(id, { ...col, originalIndex: index });
+ });
Review Comment:
### Unused Map creation with unnecessary object spreading <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The currentMap variable is created but never used in the subsequent logic, representing wasted computation and memory allocation.
###### Why this matters
This creates unnecessary overhead by building a Map with spread operations and index tracking that serves no purpose in the algorithm, consuming CPU cycles and memory without benefit.
###### Suggested change ∙ *Feature Preview*
Remove the unused currentMap creation entirely, as shown in the previous solution where only the newColumnsMap is needed for efficient lookups.
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2a6f176-dd26-4ccf-8db8-9f1bba7aa254)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:94fe7ad6-59b1-47d4-a4bb-137c0e6d8f20 -->
[](94fe7ad6-59b1-47d4-a4bb-137c0e6d8f20)
##########
superset-frontend/src/components/Datasource/components/CollectionTable/index.tsx:
##########
@@ -197,11 +195,21 @@ export default class CRUDCollection extends PureComponent<
}
changeCollection(collection: any) {
- const newCollectionArray = createCollectionArray(collection);
- this.setState({ collection, collectionArray: newCollectionArray });
+ // Preserve existing order instead of recreating from Object.keys()
+ // Map existing items to their updated versions from the collection
+ const newCollectionArray = this.state.collectionArray
+ .map(existingItem => collection[existingItem.id] || existingItem)
+ .filter(item => collection[item.id]); // Remove deleted items
+
+ // Add any new items that weren't in the existing array
+ const existingIds = new Set(this.state.collectionArray.map(item => item.id));
+ const newItems = Object.values(collection).filter((item: any) => !existingIds.has(item.id));
+ const finalCollectionArray = [...newCollectionArray, ...newItems];
Review Comment:
### Inefficient array operations in changeCollection <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The changeCollection method creates multiple intermediate arrays and performs redundant iterations over the collection data.
###### Why this matters
This approach creates unnecessary memory allocations and performs O(n) operations multiple times, which could impact performance with large datasets and frequent updates.
###### Suggested change ∙ *Feature Preview*
Combine the operations into a single pass to reduce iterations and memory allocations:
```typescript
changeCollection(collection: any) {
const existingIds = new Set(this.state.collectionArray.map(item => item.id));
const newCollectionArray: CollectionItem[] = [];
// First pass: preserve existing order and update items
for (const existingItem of this.state.collectionArray) {
if (collection[existingItem.id]) {
newCollectionArray.push(collection[existingItem.id]);
}
}
// Second pass: add new items
for (const item of Object.values(collection) as CollectionItem[]) {
if (!existingIds.has(item.id)) {
newCollectionArray.push(item);
}
}
this.setState({ collection, collectionArray: newCollectionArray });
if (this.props.onChange) {
this.props.onChange(newCollectionArray);
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/58caa36b-8f17-423b-86e2-965948c77967)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:5dbd0e31-de29-4b3c-82b3-a2d857e94cdf -->
[](5dbd0e31-de29-4b3c-82b3-a2d857e94cdf)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz merged PR #35790:
URL: https://github.com/apache/superset/pull/35790
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35790:
URL: https://github.com/apache/superset/pull/35790#issuecomment-3430946209
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](b56f845c-59b3-4bcb-b498-31481f246184)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(DatasourceEditor): preserve calculated column order when editing sql [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #35790:
URL: https://github.com/apache/superset/pull/35790#issuecomment-3450841281
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/18839343617) for [108099c](https://github.com/apache/superset/commit/108099c)
• **Environment:** http://44.248.27.128:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org