bito-code-review[bot] commented on code in PR #43757: URL: https://github.com/apache/superset/pull/43757#discussion_r4054869209
########## superset/migrations/versions/2026-09-19_00-00_a7f3c2e91d84_add_partition_filter_mapping.py: ########## @@ -0,0 +1,101 @@ +# 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. +"""add partition filter mapping + +Adds the four columns behind the ``PARTITION_FILTER_MAPPING`` feature: + +- ``tables.partition_column`` -- the physical column the engine partitions on +- ``tables.partition_mapped_column`` -- explicit override for the column whose + filters are mirrored; NULL means "follow ``main_dttm_col``" +- ``table_columns.partition_value_transform`` -- the ``:value`` expression +- ``table_columns.partition_transform_is_monotonic`` -- gates range operators + +The Continuum shadow tables get the same columns so dataset version history and +restore keep working. + +Revision ID: a7f3c2e91d84 +Revises: 60f94cd6cd11 +Create Date: 2026-08-31 22:30:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "a7f3c2e91d84" +down_revision = "60f94cd6cd11" + + +def upgrade(): Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing return type hint</b></div> <div id="fix"> Repo rule [12696] mandates explicit return type annotations on all new functions, including migration functions, but `upgrade` omits `-> None`. Sibling 2026 migrations such as `8f3a1b2c4d5e_shadow_live_row_indexes` already declare `def upgrade() -> None:` (32 of 48 migrations dated 2026 include it). Adding the annotation keeps this new file aligned with the mandated typing standard. </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/migrations/versions/2026-09-19_00-00_a7f3c2e91d84_add_partition_filter_mapping.py: ########## @@ -0,0 +1,101 @@ +# 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. +"""add partition filter mapping + +Adds the four columns behind the ``PARTITION_FILTER_MAPPING`` feature: + +- ``tables.partition_column`` -- the physical column the engine partitions on +- ``tables.partition_mapped_column`` -- explicit override for the column whose + filters are mirrored; NULL means "follow ``main_dttm_col``" +- ``table_columns.partition_value_transform`` -- the ``:value`` expression +- ``table_columns.partition_transform_is_monotonic`` -- gates range operators + +The Continuum shadow tables get the same columns so dataset version history and +restore keep working. + +Revision ID: a7f3c2e91d84 +Revises: 60f94cd6cd11 +Create Date: 2026-08-31 22:30:00.000000 + +""" + +import sqlalchemy as sa + +from superset.migrations.shared.utils import add_columns, drop_columns + +# revision identifiers, used by Alembic. +revision = "a7f3c2e91d84" +down_revision = "60f94cd6cd11" + + +def upgrade(): + add_columns( + "tables", + sa.Column("partition_column", sa.String(250), nullable=True), + sa.Column("partition_mapped_column", sa.String(250), nullable=True), + ) + add_columns( + "table_columns", + sa.Column("partition_value_transform", sa.Text(), nullable=True), + # Nullable, like the other booleans on this table. The legacy + # datasource editor saves through `update_from_object`, which writes + # NULL for any field its payload omits; NOT NULL here fails that save. + # Readers coerce with `bool(...)`, so NULL reads as "not declared" and + # ranges stop mirroring rather than mirroring unsoundly. + sa.Column( + "partition_transform_is_monotonic", + sa.Boolean(), + nullable=True, + server_default=sa.false(), + ), + ) + + # Shadow tables are nullable throughout -- a version row records the state + # of the columns that changed, so every column has to tolerate NULL. + add_columns( + "tables_version", + sa.Column("partition_column", sa.String(250), nullable=True), + sa.Column("partition_mapped_column", sa.String(250), nullable=True), + ) + add_columns( + "table_columns_version", + sa.Column("partition_value_transform", sa.Text(), nullable=True), + sa.Column("partition_transform_is_monotonic", sa.Boolean(), nullable=True), + ) + + +def downgrade(): Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing return type hint</b></div> <div id="fix"> Same rule [12696] violation as `upgrade`: `downgrade` omits the explicit `-> None` return annotation required for migration functions. Sibling migrations (e.g. `8f3a1b2c4d5e_shadow_live_row_indexes`) declare `def downgrade() -> None:`. Adding it keeps both entry points in this new file consistent with the mandated standard. </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/datasets/schemas.py: ########## @@ -468,6 +487,8 @@ def validate_unique_child_uuids(self, data: dict[str, Any], **kwargs: Any) -> No external_url = fields.String(allow_none=True) normalize_columns = fields.Boolean(load_default=False) always_filter_main_dttm = fields.Boolean(load_default=False) + partition_column = fields.String(allow_none=True) + partition_mapped_column = fields.String(allow_none=True) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing length validation</b></div> <div id="fix"> `partition_column`/`partition_mapped_column` here lack the `Length(0, 250)` validation applied to the same fields in `DatasetPostSchema` (lines 192-193) and `DatasetPutSchema` (lines 209-210). The DB columns are `String(250)` (migration a7f3c2e91d84), so an over-long value in an imported bundle passes schema validation and fails later at the DB layer with a data-error instead of a clean 400-style validation message. </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/datasets/schemas.py: ########## @@ -346,6 +361,10 @@ def fix_extra(self, data: dict[str, Any], **kwargs: Any) -> dict[str, Any]: datetime_format = fields.String( allow_none=True, validate=[Length(1, 100), validate_python_date_format] ) + partition_value_transform = fields.String(allow_none=True) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Unbounded transform on import</b></div> <div id="fix"> `partition_value_transform` in `ImportV1ColumnSchema` has no length bound, while the sibling date-format fields in the same schema use `Length(1, 255)`/`Length(1, 100)`. The transform is a SQL expression later parsed and executed (`validate_stored_expression` in `DatasetChangeCommand`), so an unbounded imported string is accepted here and only fails downstream. A bound keeps import-time rejection consistent with the PUT schema's field-level validation. </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/dashboards/schemas.py: ########## @@ -375,6 +375,9 @@ class DashboardDatasetSchema(Schema): granularity_sqla = fields.List(fields.List(fields.Str())) normalize_columns = fields.Bool() always_filter_main_dttm = fields.Bool() + partition_column = fields.Str() + partition_mapped_column = fields.Str() + partition_filter_mapping = fields.Dict(allow_none=True) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CWE-200: Dataset Schema Exposure</b></div> <div id="fix"> These fields expose dataset schema/query-construction details (column names, partition mapping summary) but are absent from both narrowing mechanisms: the guest pop-list in `DashboardDatasetSchema.post_dump` and `DASHBOARD_DATASET_INACCESSIBLE_FIELDS` in `superset/dashboards/api.py`. Callers who cannot access the datasource still receive them, contradicting that tuple's stated policy of dropping everything describing schema and query construction. Add the three field names to both lists. ([CWE-200](https://cwe.mitre.org/data/definitions/200.html)) </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/sql/parse.py: ########## @@ -1009,6 +1009,44 @@ class SQLStatement(BaseSQLStatement[exp.Expression]): } ) + def get_niladic_functions(self) -> set[str]: + """ + Names of functions called with no arguments. + + Some functions mean something entirely different with an empty argument + list: on Hive and Impala ``unix_timestamp()`` is the current time while + ``unix_timestamp(x)`` is a pure conversion. Callers that care about + determinism have to tell those apart by arity, so a name-based check + like ``check_functions_present`` is not enough. + """ + niladic: set[str] = set() + for function in self._parsed.find_all(exp.Func): + sql_name = function.sql_name() + name = function.name.upper() if sql_name == "ANONYMOUS" else sql_name Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Duplicated name-resolution logic</b></div> <div id="fix"> This name-resolution idiom (`sql_name()` plus the `ANONYMOUS` branch) duplicates `check_functions_present` (lines 1484-1497), and the two copies must stay in sync for consistent function-name reporting. Consider extracting a shared `_function_sql_name(function)` helper used by both loops; that also removes the `.upper()` applied twice (lines 1025 and 1027). </div> </div> <small><i>Code Review Run #95ed46</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 ########## superset/commands/dataset/update.py: ########## @@ -399,6 +405,103 @@ def _validate_expressions( ) ) + def _validate_partition_mapping(self, exceptions: list[ValidationError]) -> None: + """ + Validate the dataset's partition filter mapping. + + Only the blocking (Tier 1) issues become validation errors. Tier 2 + issues -- an unparseable transform, a transform missing `:value` -- + deliberately let the save through and leave the mapping inactive, per + the PRD, so a half-written transform doesn't cost the owner the rest of + their edits. They are surfaced by the editor, not by rejecting the PUT. + + The transform is authored by a dataset owner, the same principal and + trust level as a calculated-column expression, so it also goes through + `validate_stored_expression` -- the parser gate that already governs + stored expressions. + """ + self._model = cast(SqlaTable, self._model) + + columns = self._properties.get("columns") + column_names = ( + {column["column_name"] for column in columns} + if columns is not None + else {column.column_name for column in self._model.columns} + ) + + partition_column = self._properties.get( + "partition_column", self._model.partition_column + ) + partition_mapped_column = self._properties.get( + "partition_mapped_column", self._model.partition_mapped_column + ) + main_dttm_col = self._properties.get("main_dttm_col", self._model.main_dttm_col) + if not partition_column: + return + + database = self._properties.get("database") or self._model.database + catalog = self._properties.get("catalog", self._model.catalog) + schema = self._properties.get("schema", self._model.schema) + + effective_mapped_column = partition_mapped_column or main_dttm_col Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Effective-column rule triplicated</b></div> <div id="fix"> `effective_mapped_column = partition_mapped_column or main_dttm_col` re-derives the fallback rule that `validate_partition_mapping` (partition_mapping.py:232) and `SqlaTable.partition_filter_mapping_summary` (models.py:1952) already encode -- this diff adds a third copy. If the fallback rule changes, the transform lookup here and the self-mapping check in the callee diverge silently. Consider one `effective_mapped_column()` helper in `partition_mapping` used by all three. </div> </div> <small><i>Code Review Run #95ed46</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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
