You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "fitzee (via GitHub)" <gi...@apache.org> on 2026/06/30 21:12:50 UTC
[PR] fix(migration): strip metricSqlExpressions from ag_grid_table params [superset]
fitzee opened a new pull request, #41591:
URL: https://github.com/apache/superset/pull/41591
### SUMMARY
Follow-up to #41555, which stopped *new* writes of `metricSqlExpressions` into `extra_form_data`. However, charts saved before that fix still carry the bloated data in their `params` and `query_context` columns — for datasources with many metrics this can be hundreds of MB per chart, loaded eagerly on every dashboard request.
This migration strips `metricSqlExpressions` from `extra_form_data` in both columns for all `ag_grid_table` slices.
**Root cause recap**: The AG Grid table plugin puts `metricSqlExpressions` (a `Record<string, string>` mapping every datasource metric/column to its SQL expression) into `ownState` on filter interactions. Before #41555, `ExploreViewContainer` spread all of `ownState` into `extra_form_data`, which was then serialised into `params` on save.
**What this migration does**:
- Queries all slices with `viz_type = 'ag_grid_table'`
- Removes `metricSqlExpressions` from `params.extra_form_data`
- Removes `metricSqlExpressions` from `query_context.form_data.extra_form_data`
- Commits in a single transaction
**Downgrade**: no-op — the field is runtime-derived by `transformProps` on each render and does not need to be restored.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — data migration only.
### TESTING INSTRUCTIONS
1. Find an `ag_grid_table` slice that was saved with filters applied before #41555 (or manually insert `metricSqlExpressions` into `params.extra_form_data` to simulate)
2. Run `superset db upgrade`
3. Confirm `metricSqlExpressions` is absent from `params` for all `ag_grid_table` slices
4. Confirm charts still load and filter correctly (the field is regenerated at runtime)
5. Run `superset db downgrade` — migration is a no-op and succeeds cleanly
### ADDITIONAL INFORMATION
- [x] Has associated issue: closes #41555 (follow-up)
- [ ] Required feature flags:
- [ ] Changes UI
- [x] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [x] 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
**Runtime estimate**: O(n) over `ag_grid_table` slices. Reads and rewrites `params`/`query_context` in Python — for a typical installation with tens to hundreds of such slices this completes in seconds. For very large installations (thousands of slices) consider running during a maintenance window.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4853255764
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 28482049c5976547b674e1a7e77a8457ea4ae4bb |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a44e8108e68c700079dd169 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41591--superset-docs-preview.netlify.app](https://deploy-preview-41591--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DctLCoMwEADQu8y6wQgSY2_gMcZkxEA-44yCpfTuze5t3hdYWuEL3gAv2ARrOLr39AwhE9abTaFLUtAz08NCqqlVNXtfhlGwaH-ROLfPGvt0OE3kR-vJ-TBba-clxtEt8PsD) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41591--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3504194427
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,116 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 78a40c08b4be
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "78a40c08b4be"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE).all()
+ changed = 0
+ for slc in slices:
+ dirty = _strip_params(slc)
+ dirty = _strip_query_context(slc) or dirty
+ if dirty:
+ changed += 1
+
+ if changed:
+ session.commit()
Review Comment:
a `commit` on a migration version will break a possible rollback for the entire migration. Explicitly commits that transaction mid-migration. If anything fails after this point
(e.g. a later migration in the same alembic upgrade head run, or even a crash after the commit but before Alembic records the revision in alembic_version), you get a partially-applied state that can't be rolled back.
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,116 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 78a40c08b4be
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "78a40c08b4be"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE).all()
+ changed = 0
+ for slc in slices:
+ dirty = _strip_params(slc)
+ dirty = _strip_query_context(slc) or dirty
+ if dirty:
+ changed += 1
+
+ if changed:
+ session.commit()
+ session.close()
Review Comment:
let's remove the close, alembic owns the lifecycle of the session
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3582562193
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In Alembic migrations, use `session.flush()` within the migration transaction instead of `session.commit()`; do not flag missing commits when the migration is flushed under Alembic-managed transaction control.
**Applied to:**
- `**/migration*/**`
- `**/*migration*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3582561187
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
<!-- Bito Reply -->
The reviewer's concern regarding the missing commit has been addressed in the current version of the migration. The `upgrade()` function now includes a `session.commit()` call that executes only if changes were made to the records, which is the correct pattern for Alembic migrations to ensure atomicity and proper rollback capabilities.
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3505024828
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,116 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 78a40c08b4be
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "78a40c08b4be"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE).all()
+ changed = 0
+ for slc in slices:
+ dirty = _strip_params(slc)
+ dirty = _strip_query_context(slc) or dirty
+ if dirty:
+ changed += 1
+
+ if changed:
+ session.commit()
Review Comment:
Good catch — removed. Replaced the conditional commit + close with a plain loop that lets Alembic manage the transaction lifecycle. Fixed in 2848204.
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,116 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 78a40c08b4be
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "78a40c08b4be"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ slices = session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE).all()
+ changed = 0
+ for slc in slices:
+ dirty = _strip_params(slc)
+ dirty = _strip_query_context(slc) or dirty
+ if dirty:
+ changed += 1
+
+ if changed:
+ session.commit()
+ session.close()
Review Comment:
Done — removed in the same commit (2848204).
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4855330135
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3576892635
##########
tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,200 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Tests for migration ``d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params``.
+
+Covers the two helper functions (_strip_params, _strip_query_context) and the
+full upgrade() path that seeds an ag_grid_table slice with metricSqlExpressions
+in both params and query_context, then asserts both fields are stripped.
+"""
+
+from __future__ import annotations
+
+import json as stdlib_json
+from importlib import import_module
+from unittest.mock import patch
+
+import pytest
+from sqlalchemy import create_engine
+from sqlalchemy.orm import Session
+
+migration = import_module(
+ "superset.migrations.versions."
+ "2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params"
+)
+
+Slice = migration.Slice
+_strip_params = migration._strip_params
+_strip_query_context = migration._strip_query_context
+_FIELD = migration._FIELD
+_VIZ_TYPE = migration._VIZ_TYPE
+
+_SQL_EXPR = {"col1": "SELECT col1 FROM t", "col2": "SUM(revenue)"}
+
+
+def _contaminated_params() -> str:
+ return stdlib_json.dumps(
+ {
+ "viz_type": _VIZ_TYPE,
+ "extra_form_data": {
+ _FIELD: _SQL_EXPR,
+ "filters": [{"col": "col1", "op": "==", "val": "a"}],
+ },
+ }
+ )
+
+
+def _contaminated_query_context() -> str:
+ return stdlib_json.dumps(
+ {
+ "form_data": {
+ "extra_form_data": {
+ _FIELD: _SQL_EXPR,
+ },
+ },
+ "queries": [],
+ }
+ )
+
+
+@pytest.fixture
+def engine():
+ engine = create_engine("sqlite:///:memory:")
+ migration.Base.metadata.create_all(engine)
+ return engine
+
+
+# ---------------------------------------------------------------------------
+# Helper-function unit tests
+# ---------------------------------------------------------------------------
+
+
+def test_strip_params_removes_field() -> None:
+ slc = Slice(params=_contaminated_params())
+ changed = _strip_params(slc)
+ assert changed
+ result = stdlib_json.loads(slc.params)
+ assert _FIELD not in result["extra_form_data"]
+ assert result["extra_form_data"]["filters"] == [{"col": "col1", "op": "==", "val": "a"}]
+
+
+def test_strip_params_noop_when_field_absent() -> None:
+ params = stdlib_json.dumps({"extra_form_data": {"filters": []}})
+ slc = Slice(params=params)
+ changed = _strip_params(slc)
+ assert not changed
+ assert stdlib_json.loads(slc.params) == {"extra_form_data": {"filters": []}}
+
+
+def test_strip_params_noop_when_params_empty() -> None:
+ assert not _strip_params(Slice(params=None))
+
+
+def test_strip_params_noop_on_invalid_json() -> None:
+ slc = Slice(params="not-json")
+ changed = _strip_params(slc)
+ assert not changed
+ assert slc.params == "not-json"
+
+
+def test_strip_query_context_removes_field() -> None:
+ slc = Slice(query_context=_contaminated_query_context())
+ changed = _strip_query_context(slc)
+ assert changed
+ result = stdlib_json.loads(slc.query_context)
+ assert _FIELD not in result["form_data"]["extra_form_data"]
+
+
+def test_strip_query_context_noop_when_field_absent() -> None:
+ qc = stdlib_json.dumps({"form_data": {"extra_form_data": {}}})
+ slc = Slice(query_context=qc)
+ assert not _strip_query_context(slc)
+
+
+def test_strip_query_context_noop_when_query_context_empty() -> None:
+ assert not _strip_query_context(Slice(query_context=None))
+
+
+def test_strip_query_context_noop_on_invalid_json() -> None:
+ slc = Slice(query_context="not-json")
+ changed = _strip_query_context(slc)
+ assert not changed
+ assert slc.query_context == "not-json"
+
+
+# ---------------------------------------------------------------------------
+# Full upgrade() integration test
+# ---------------------------------------------------------------------------
+
+
+def test_upgrade_strips_both_fields(engine) -> None:
Review Comment:
**Suggestion:** Add a type annotation for the function parameter to satisfy the rule requiring typed function signatures. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The test function takes the `engine` fixture parameter without a type annotation, and the rule requires type hints on function parameters when they can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c4b0d328f02e4da8b29aa5d60c34b878&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=c4b0d328f02e4da8b29aa5d60c34b878&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 143:143
**Comment:**
*Custom Rule: Add a type annotation for the function parameter to satisfy the rule requiring typed function signatures.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=c708c9211a7bd07373aa2ec2cfbb77a2c6f54520ddebd57f9e0c6f849b342672&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=c708c9211a7bd07373aa2ec2cfbb77a2c6f54520ddebd57f9e0c6f849b342672&reaction=dislike'>👎</a>
##########
tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,200 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Tests for migration ``d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params``.
+
+Covers the two helper functions (_strip_params, _strip_query_context) and the
+full upgrade() path that seeds an ag_grid_table slice with metricSqlExpressions
+in both params and query_context, then asserts both fields are stripped.
+"""
+
+from __future__ import annotations
+
+import json as stdlib_json
+from importlib import import_module
+from unittest.mock import patch
+
+import pytest
+from sqlalchemy import create_engine
+from sqlalchemy.orm import Session
+
+migration = import_module(
+ "superset.migrations.versions."
+ "2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params"
+)
+
+Slice = migration.Slice
+_strip_params = migration._strip_params
+_strip_query_context = migration._strip_query_context
+_FIELD = migration._FIELD
+_VIZ_TYPE = migration._VIZ_TYPE
+
+_SQL_EXPR = {"col1": "SELECT col1 FROM t", "col2": "SUM(revenue)"}
+
+
+def _contaminated_params() -> str:
+ return stdlib_json.dumps(
+ {
+ "viz_type": _VIZ_TYPE,
+ "extra_form_data": {
+ _FIELD: _SQL_EXPR,
+ "filters": [{"col": "col1", "op": "==", "val": "a"}],
+ },
+ }
+ )
+
+
+def _contaminated_query_context() -> str:
+ return stdlib_json.dumps(
+ {
+ "form_data": {
+ "extra_form_data": {
+ _FIELD: _SQL_EXPR,
+ },
+ },
+ "queries": [],
+ }
+ )
+
+
+@pytest.fixture
+def engine():
+ engine = create_engine("sqlite:///:memory:")
+ migration.Base.metadata.create_all(engine)
+ return engine
Review Comment:
**Suggestion:** Add a return type annotation to this fixture function so it complies with the required type-hint rule. [custom_rule]
**Severity Level:** Minor 🧹
<details>
<summary><b>Why it matters? ⭐ </b></summary>
This fixture function is newly added and omits a return type annotation, which violates the rule requiring type hints on functions that can be annotated.
</details>
<details>
<summary><b>Rule source 📖 </b></summary>
.cursor/rules/dev-standard.mdc (line 28)
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1eb9f681e35e40cc935fd7c3a6f815b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1eb9f681e35e40cc935fd7c3a6f815b3&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** tests/unit_tests/migrations/test_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 73:77
**Comment:**
*Custom Rule: Add a return type annotation to this fixture function so it complies with the required type-hint rule.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=adf21d7edfae04a4f7fe182bbc342da86d612cdfa5ea002cd2fdd308dd2e5f20&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=adf21d7edfae04a4f7fe182bbc342da86d612cdfa5ea002cd2fdd308dd2e5f20&reaction=dislike'>👎</a>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4970160563
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4859700203
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4848093167
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41591?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 69.52%. Comparing base ([`bf88c62`](https://app.codecov.io/gh/apache/superset/commit/bf88c6281495eb0b6db62b001a0b2313b33f04a6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`a5867b8`](https://app.codecov.io/gh/apache/superset/commit/a5867b85bfc35dd0383014dcb7d4bf899ef2bf31?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41591 +/- ##
==========================================
+ Coverage 64.50% 69.52% +5.02%
==========================================
Files 2666 1866 -800
Lines 146864 78859 -68005
Branches 33910 25404 -8506
==========================================
- Hits 94728 54828 -39900
+ Misses 50421 24031 -26390
+ Partials 1715 0 -1715
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [mysql](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [postgres](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [python](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+40.72%)` | :arrow_up: |
| [sqlite](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit](https://app.codecov.io/gh/apache/superset/pull/41591/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (ø)` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41591?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3509946291
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
Review Comment:
**Suggestion:** This code assumes decoded `params` is always a dict; if any legacy row contains valid JSON that is not an object (e.g. list/string), `.get(...)` raises and aborts the whole migration. Guard with a dict type check before accessing keys so malformed-but-parseable rows are safely skipped. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ superset db upgrade can fail on malformed params.
- ⚠️ One bad slice blocks cleaning remaining ag_grid_table charts.
- ⚠️ Migration robustness reduced for historical params JSON payloads.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `upgrade()` (lines 96–102) in
`superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`
iterates over all ag_grid_table slices and calls `_strip_params(slc)` (line 101).
2. `_strip_params` (lines 59–75) loads JSON from `slc.params` using `params =
json.loads(slc.params)` inside a `try` block; any parseable JSON value, including non-dict
structures, will be stored in `params` without error.
3. The function immediately does `extra = params.get("extra_form_data", {})` at line 68;
if `params` is not a dict (e.g. a list or string from legacy or corrupted data), this
`.get` call raises `AttributeError` because those types do not implement `get`.
4. This exception is not caught (only `json.loads` is wrapped in `try/except`), so the
error propagates out of `_strip_params`, aborting the `for` loop in `upgrade()` and
causing `superset db upgrade` to fail whenever a slice’s `params` column contains valid
JSON that is not a dict.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=702cd8d3ffd74d77b92de27bfbc24f6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=702cd8d3ffd74d77b92de27bfbc24f6e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 63:68
**Comment:**
*Type Error: This code assumes decoded `params` is always a dict; if any legacy row contains valid JSON that is not an object (e.g. list/string), `.get(...)` raises and aborts the whole migration. Guard with a dict type check before accessing keys so malformed-but-parseable rows are safely skipped.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=33aa1c260b2f0f82545fe08fb4e97e97903d612b1ae441859f3621f8d9eaea16&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=33aa1c260b2f0f82545fe08fb4e97e97903d612b1ae441859f3621f8d9eaea16&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
Review Comment:
**Suggestion:** This logic assumes decoded `query_context` is a dict with nested dicts; if a row contains a different JSON shape, `.get(...)` may raise and stop the migration. Add structure/type checks before nested access so unexpected historical payloads do not fail the entire upgrade. [type error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ superset db upgrade may crash on malformed query_context.
- ⚠️ Single corrupted slice prevents cleaning other ag_grid_table slices.
- ⚠️ Less robust than prior query_context repair migration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. `upgrade()` (lines 96–102) iterates over ag_grid_table slices and calls
`_strip_query_context(slc)` for each row (line 102) in
`superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`.
2. `_strip_query_context` (lines 78–93) loads JSON from `slc.query_context` with `qc =
json.loads(slc.query_context)` inside a `try` block; any parseable JSON value, including
non-dict shapes, will be assigned to `qc`.
3. The function then performs nested dictionary access via `extra = qc.get("form_data",
{}).get("extra_form_data", {})` at line 87; if `qc` is not a dict or `qc["form_data"]` is
not a dict (for example, a string or list), this chained `.get` call raises
`AttributeError` or `TypeError`, which are not caught.
4. Superset’s existing migration `fix_form_data_string_in_query_context` at
`superset/migrations/versions/2025-12-16_12-00_f5b5f88d8526_fix_form_data_string_in_query_context.py:29–63`
demonstrates that `query_context` and its `form_data` field have historically been stored
with unexpected shapes (e.g. `form_data` as a JSON string) and guards nested access with
broad exception handling, so this migration’s unvalidated nested `.get` chain can
realistically terminate `superset db upgrade` when it encounters similarly irregular
`query_context` payloads.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bc96aa8d5f7e436fb4bf981d1b65c3d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bc96aa8d5f7e436fb4bf981d1b65c3d5&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 82:88
**Comment:**
*Type Error: This logic assumes decoded `query_context` is a dict with nested dicts; if a row contains a different JSON shape, `.get(...)` may raise and stop the migration. Add structure/type checks before nested access so unexpected historical payloads do not fail the entire upgrade.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=3bd6072c5f0169fb57f2b16413885003bed089a56434b684837b7b3b6cb7aa86&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=3bd6072c5f0169fb57f2b16413885003bed089a56434b684837b7b3b6cb7aa86&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
**Suggestion:** The migration mutates ORM objects but never commits the session, so the stripped fields may never be persisted to the database. Add an explicit `session.commit()` (and ideally close the session) after processing rows to ensure updates are written. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ AG Grid table slices keep oversized metricSqlExpressions blobs.
- ❌ Dashboard load still reads inflated slice params JSON.
- ⚠️ Migration appears successful but leaves data unchanged.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run `superset db upgrade`, which executes migration
`d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params` in
`superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`.
2. In `upgrade()` (lines 96–102), a SQLAlchemy session is created via `session =
db.Session(bind=bind)` and all `Slice` rows with `viz_type == "ag_grid_table"` are
iterated, mutating `slc.params` and `slc.query_context` through `_strip_params` and
`_strip_query_context`.
3. Unlike other Superset data migrations that use ORM sessions and explicitly commit (for
example `country_map_use_lowercase_country_name.upgrade()` at
`versions/2021-04-09_16-14_085f06488938_country_map_use_lowercase_country_name.py:48–57`
which calls `session.commit()` and `session.close()` at lines 16–17), this migration
returns from `upgrade()` without any `session.commit()` or `session.close()` call.
4. Because the session’s dirty objects are never flushed or committed, the in-memory
changes to `params` and `query_context` are discarded when the migration finishes, leaving
`metricSqlExpressions` present in the database for all ag_grid_table slices despite the
migration having run.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=686d537c4a7f4b34a2e494175b4b30c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=686d537c4a7f4b34a2e494175b4b30c0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 96:102
**Comment:**
*Incomplete Implementation: The migration mutates ORM objects but never commits the session, so the stripped fields may never be persisted to the database. Add an explicit `session.commit()` (and ideally close the session) after processing rows to ensure updates are written.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=59c21c470289e14e91d21c84b2b08aec1261e214289f8272ccfb0dd795049208&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=59c21c470289e14e91d21c84b2b08aec1261e214289f8272ccfb0dd795049208&reaction=dislike'>👎</a>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4847978804
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 02c6e5a043abbeb077915796f51b522ea7ae38a3 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/6a44315399525f00086cd276 |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-41591--superset-docs-preview.netlify.app](https://deploy-preview-41591--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
|<span aria-hidden="true">🤖</span> Make changes | [Run an agent on this branch](https://app.netlify.com/projects/superset-docs-preview/agent-runs#DcvLDcIwDADQXXwmIrRNoGzQMUziiEj5GLtIRYjdye1d3hdYeuUd7gAneAi28BxO-TiHQtjebCrtkoO-Ch0spJp7U5PGMoyCVceLxKV_tjimx2WZL25eVze5ZK29-RCnq4ffHw) |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-41591--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4968193322
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3582560643
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
Review Comment:
Not a realistic concern for this codebase. Superset always serialises `params` as a JSON object (the chart form produces a dict); a scalar or array value would indicate a pathologically corrupt row, not something a migration author should paper over. `_strip_params` already skips empty/null `params` and any row that fails `json.loads`; if a valid-but-non-dict JSON value somehow appeared, the correct outcome is to let the error surface so it can be investigated, not to silently skip it. Treating this as critical is a false positive.
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
Review Comment:
Same reasoning as the `params` thread above: Superset always serialises `query_context` as a JSON object with a `form_data` dict. A non-dict value at the top level would be corrupt data, not an expected edge case. The existing `try/except` around `json.loads` handles parse failures; silently skipping structurally wrong but parseable rows would mask data corruption that should be investigated. False positive.
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3582563358
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In this migration, do not add extra defensive type checks for `params` beyond JSON parsing; if a valid but non-dict JSON value appears, let the error surface rather than silently skipping it.
**Applied to:**
- `superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
Review Comment:
✅ **Customized review instruction saved!**
**Instruction:**
> In migration scripts, do not add defensive structure/type checks to silently skip parseable-but-malformed `query_context` payloads; treat structurally invalid but parseable rows as data corruption that should be surfaced instead of masked.
**Applied to:**
- `**/migration*/**`
- `**/*migration*.<ext>`
---
💡 *To manage or update this instruction, visit: [CodeAnt AI Settings](https://app.codeant.ai/org/settings/learnings)*
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4847964201
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4861520520
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - 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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "fitzee (via GitHub)" <gi...@apache.org>.
fitzee commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3582559160
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
Stale — the current code uses `session.flush()` (not `session.commit()`) at the end of `upgrade()`, added in response to Joe's review. Calling `session.commit()` inside a migration would break Alembic's transaction management and prevent rollback; `flush()` is the correct pattern here — it sends the pending SQL UPDATEs within the outer transaction that Alembic owns.
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
Stale — the bot scanned an earlier snapshot. The current code has `session.flush()` at the end of `upgrade()`, added in response to Joe's review. Using `session.commit()` here would be incorrect: it would commit mid-migration and break Alembic's ability to roll back if a later step fails. `session.flush()` is the right pattern — it sends the pending SQL UPDATEs inside the outer transaction that Alembic owns and commits.
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41591:
URL: https://github.com/apache/superset/pull/41591#issuecomment-4972469646
@fitzee there are still some bot review comments marked as critical... can you assess these and either apply or refute the suggestions on those threads?
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "rebenitez1802 (via GitHub)" <gi...@apache.org>.
rebenitez1802 merged PR #41591:
URL: https://github.com/apache/superset/pull/41591
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3507845720
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: a7d3f1b9c2e4
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "a7d3f1b9c2e4"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
Review Comment:
**Suggestion:** Use the shared migration JSON-loading helper for `query_context` parsing to keep migration behavior consistent and compatibility-focused. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The migration utilities provide `try_load_json` for safe JSON loading in migrations, but this code performs manual parsing with a broad exception handler instead. That is the kind of ad hoc logic the shared-utils rule is meant to avoid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3a7262c3758d4fee985edf894d18e1d2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=3a7262c3758d4fee985edf894d18e1d2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 82:85
**Comment:**
*Custom Rule: Use the shared migration JSON-loading helper for `query_context` parsing to keep migration behavior consistent and compatibility-focused.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=eca45bbcb14d174e6e55e4b9bfa958634ddd8e4e4b1fc75305df2b66043831ed&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=eca45bbcb14d174e6e55e4b9bfa958634ddd8e4e4b1fc75305df2b66043831ed&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: a7d3f1b9c2e4
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "a7d3f1b9c2e4"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
Review Comment:
**Suggestion:** Use the shared JSON-loading helper from migration utilities instead of manual JSON parsing with broad exception handling. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The shared migration utilities include `try_load_json`, which is designed for this exact pattern. Since the migration manually parses JSON with broad exception handling instead of using the helper, this is a real shared-utils violation.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=99daf0584d214a3a8ee1bdf6ebd13614&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=99daf0584d214a3a8ee1bdf6ebd13614&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 63:66
**Comment:**
*Custom Rule: Use the shared JSON-loading helper from migration utilities instead of manual JSON parsing with broad exception handling.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=d384a6f681bba7a8d0da9a94cd15ebfe9d665ccbb5c19d1dc668d7c8c5d0a50b&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=d384a6f681bba7a8d0da9a94cd15ebfe9d665ccbb5c19d1dc668d7c8c5d0a50b&reaction=dislike'>👎</a>
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: a7d3f1b9c2e4
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "a7d3f1b9c2e4"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data. Returns True if changed."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
**Suggestion:** Use the shared migration pagination helper instead of iterating directly over a raw ORM query, so the migration follows the project’s compatibility-safe migration utilities pattern. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The migration iterates a raw ORM query directly, while the project has a shared compatibility-safe batching helper (`paginated_update`) used in other migrations. This matches the migration-use-shared-utils rule, so the warning is valid.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=76b52f496e4e42beabf8128b6e3ec672&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=76b52f496e4e42beabf8128b6e3ec672&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py
**Line:** 96:102
**Comment:**
*Custom Rule: Use the shared migration pagination helper instead of iterating directly over a raw ORM query, so the migration follows the project’s compatibility-safe migration utilities pattern.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=9886b1e954acff101ec51a4e2731a1d37684eedc080eae01c60846c19a36b180&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41591&comment_hash=9886b1e954acff101ec51a4e2731a1d37684eedc080eae01c60846c19a36b180&reaction=dislike'>👎</a>
--
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(migration): strip metricSqlExpressions from ag_grid_table params [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41591:
URL: https://github.com/apache/superset/pull/41591#discussion_r3510006193
##########
superset/migrations/versions/2026-06-30_00-00_d24e6b0a9c7f_strip_metricsqlexpressions_from_ag_grid_params.py:
##########
@@ -0,0 +1,108 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""strip metricSqlExpressions from ag_grid_table params
+
+Before PR #41555 the AG Grid table plugin leaked ``metricSqlExpressions``
+(a mapping of every datasource metric/column name to its SQL expression) into
+``extra_form_data`` on every filter interaction, which was then serialised into
+the ``params`` and ``query_context`` columns on save. For datasources with
+many metrics this bloated each chart record by hundreds of MB.
+
+This migration strips the field from existing rows so those records are no
+longer loaded eagerly on every dashboard request.
+
+Revision ID: d24e6b0a9c7f
+Revises: 2bee73611e32
+Create Date: 2026-06-30 00:00:00.000000
+
+"""
+
+from alembic import op
+from sqlalchemy import Column, Integer, String, Text
+from sqlalchemy.ext.declarative import declarative_base
+
+from superset import db
+from superset.utils import json
+
+# revision identifiers, used by Alembic.
+revision = "d24e6b0a9c7f"
+down_revision = "2bee73611e32"
+
+Base = declarative_base()
+
+_FIELD = "metricSqlExpressions"
+_VIZ_TYPE = "ag_grid_table"
+
+
+class Slice(Base):
+ __tablename__ = "slices"
+ id = Column(Integer, primary_key=True)
+ viz_type = Column(String(250))
+ params = Column(Text)
+ query_context = Column(Text)
+
+
+def _strip_params(slc: Slice) -> bool:
+ """Remove _FIELD from extra_form_data in params. Returns True if changed."""
+ if not slc.params:
+ return False
+ try:
+ params = json.loads(slc.params)
+ except Exception:
+ return False
+
+ extra = params.get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ params["extra_form_data"] = extra
+ slc.params = json.dumps(params)
+ return True
+
+
+def _strip_query_context(slc: Slice) -> bool:
+ """Remove _FIELD from query_context.form_data.extra_form_data."""
+ if not slc.query_context:
+ return False
+ try:
+ qc = json.loads(slc.query_context)
+ except Exception:
+ return False
+
+ extra = qc.get("form_data", {}).get("extra_form_data", {})
+ if _FIELD not in extra:
+ return False
+
+ del extra[_FIELD]
+ slc.query_context = json.dumps(qc)
+ return True
+
+
+def upgrade() -> None:
+ bind = op.get_bind()
+ session = db.Session(bind=bind)
+
+ for slc in session.query(Slice).filter(Slice.viz_type == _VIZ_TYPE):
+ _strip_params(slc)
+ _strip_query_context(slc)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing database commit</b></div>
<div id="fix">
The `upgrade()` function modifies `slc.params` and `slc.query_context` but never calls `session.commit()`. All 20+ comparable migrations in this codebase that modify records call `session.commit()` after processing (e.g., `2023-07-19_17-54_ee179a490af9_deckgl_path_width_units.py:66`). Without commit, all changes are rolled back when the migration context closes.
</div>
</div>
<small><i>Code Review Run #b27f64</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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