aminghadersohi commented on code in PR #43870: URL: https://github.com/apache/superset/pull/43870#discussion_r4342087594
########## superset-frontend/src/components/Datasource/components/DatasourceEditor/components/PartitionFilterMapping/utils.test.ts: ########## @@ -0,0 +1,1082 @@ +/** + * 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. + */ +import { + applyImplicitMappingMove, + applyMappingMove, + clearMappingTransforms, + clearUnmappedTransforms, + clearUnmappedTransformsAcrossMove, + defaultTransformFor, + mappedColumnIsImplicit, + mappingIsActive, + nextMappedColumnOverride, + partitionMappingErrors, + partitionRowState, + previewOperatorFor, + sampleValuesFor, + resolveMappedColumn, + suggestedMappedColumn, + transformCanPreview, + valueTransformIssues, +} from './utils'; +import type { PartitionMappingColumn } from './types'; + +const COLUMNS = [ + { + column_name: 'event_time', + type: 'TIMESTAMP', + is_dttm: true, + filterable: true, + groupby: true, + }, + { column_name: 'dt_epoch', type: 'BIGINT', filterable: true, groupby: true }, + { column_name: 'country', type: 'TEXT', filterable: true, groupby: true }, +]; + +/** Mapping `event_time` onto the `dt_epoch` partition, with no override. */ +const MAPPED_TO_EVENT_TIME = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + partition_mapped_column: null, +}; + +test('the mapped column follows the default datetime column', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + partition_mapped_column: null, + }; + + expect(resolveMappedColumn(datasource)).toBe('event_time'); + expect(mappedColumnIsImplicit(datasource)).toBe(true); +}); + +test('an explicit override wins over the default datetime column', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'region_key', + partition_mapped_column: 'country', + }; + + expect(resolveMappedColumn(datasource)).toBe('country'); + expect(mappedColumnIsImplicit(datasource)).toBe(false); +}); + +test('there is no mapped column without a partition column', () => { + expect( + resolveMappedColumn({ + main_dttm_col: 'event_time', + partition_column: null, + }), + ).toBeNull(); +}); + +test('a partition column with no datetime column maps to nothing', () => { + // Wireframe 1g: a partition column is designated but nothing mirrors onto it. + const datasource = { main_dttm_col: null, partition_column: 'dt_epoch' }; + + expect(resolveMappedColumn(datasource)).toBeNull(); + expect(mappingIsActive(datasource, COLUMNS)).toBe(false); +}); + +test('a mapping without a transform is configured but inert', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + }; + + expect(mappingIsActive(datasource, COLUMNS)).toBe(false); +}); + +test('a mapping with a transform is active', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + }; + const columns = COLUMNS.map(column => + column.column_name === 'event_time' + ? { ...column, partition_value_transform: 'unix_timestamp(:value)' } + : column, + ); + + expect(mappingIsActive(datasource, columns)).toBe(true); +}); + +test('a whitespace-only transform does not count as active', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + }; + const columns = COLUMNS.map(column => + column.column_name === 'event_time' + ? { ...column, partition_value_transform: ' ' } + : column, + ); + + expect(mappingIsActive(datasource, columns)).toBe(false); +}); + +test('each column gets one of the three row treatments', () => { + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + }; + + expect(partitionRowState(datasource, 'event_time')).toBe('mapped'); + expect(partitionRowState(datasource, 'country')).toBe('unmapped'); + expect(partitionRowState(datasource, 'dt_epoch')).toBe('partition'); +}); + +test('no column has a row treatment without a partition column', () => { + expect(partitionRowState({ main_dttm_col: 'event_time' }, 'event_time')).toBe( + 'none', + ); +}); + +test('moving the mapping discards the previous transform', () => { + // Both cardinalities are one, so reassignment replaces rather than + // accumulating a second mapping. + const columns = [ + { + column_name: 'event_time', + is_dttm: true, + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { column_name: 'country' }, + ]; + + const moved = applyMappingMove(MAPPED_TO_EVENT_TIME, columns, 'country', ''); + + expect(moved[0]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('moving the mapping clears a transform on any column, not just the last one', () => { + // Reaching "no mapping" and then mapping a new column must not leave the + // old transform behind to come back to life later. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { column_name: 'country' }, + ]; + + const moved = applyMappingMove( + MAPPED_TO_EVENT_TIME, + columns, + 'country', + 'lower(:value)', + ); + + expect(moved[0].partition_value_transform).toBeNull(); + expect(moved[1].partition_value_transform).toBe('lower(:value)'); +}); + +test('moving the mapping pre-fills the new column when we have a default', () => { + const columns: PartitionMappingColumn[] = [ + { column_name: 'event_time', is_dttm: true }, + ]; + + const moved = applyMappingMove( + { main_dttm_col: null, partition_column: 'dt_epoch' }, + columns, + 'event_time', + 'unix_timestamp(:value)', + ); + + expect(moved[0].partition_value_transform).toBe('unix_timestamp(:value)'); +}); + +test('moving the mapping does not pick up a leftover transform', () => { + // The mapping has one mirrored column, so a transform on any *other* column + // is a leftover some writer stranded there -- not an expression written about + // this column. Picking it up made "Move mapping to this column" a live + // mapping with no typing, emitting predicates nobody authored while the + // pruning indicator stayed green: NEW-R11-01. + const columns = [ + { column_name: 'event_time', is_dttm: true }, + { + column_name: 'country', + partition_value_transform: 'lower(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyMappingMove(MAPPED_TO_EVENT_TIME, columns, 'country', ''); + + expect(moved[1]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('moving the mapping installs the engine default over a leftover', () => { + // The move is not refused, only disarmed: it arrives holding whatever the + // engine offers, which is the same thing a clean column would get. + const columns = [ + { column_name: 'event_time', is_dttm: true }, + { + column_name: 'other_time', + is_dttm: true, + partition_value_transform: 'to_unixtime(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyMappingMove( + MAPPED_TO_EVENT_TIME, + columns, + 'other_time', + 'unix_timestamp(:value)', + ); + + expect(moved[1]).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: false, + }); +}); + +test('re-selecting the column already mapped keeps its transform', () => { + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyMappingMove( + MAPPED_TO_EVENT_TIME, + columns, + 'event_time', + '', + ); + + expect(moved[0]).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }); +}); + +test('removing a mapping clears the transform on every column, not just the mapped one', () => { + // Removing the override drops the mapping back onto the default datetime + // column, so a transform left anywhere else is not dormant -- it is the next + // mapping, armed and invisible. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { + column_name: 'event_time2', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const cleared = clearMappingTransforms(columns); + + expect(cleared).toMatchObject([ + { + partition_value_transform: null, + partition_transform_is_monotonic: false, + }, + { + partition_value_transform: null, + partition_transform_is_monotonic: false, + }, + ]); +}); + +test('clearing a mapping nothing holds hands back the same columns', () => { + // Callers clear unconditionally, and column state's identity is what triggers + // the editor's validation pass -- a fresh array every time would run it on + // every unrelated edit. + const columns = [{ column_name: 'event_time' }, { column_name: 'country' }]; + + expect(clearMappingTransforms(columns)).toBe(columns); +}); + +test('a transform on a column the mapping does not mirror is cleared', () => { + // The editor is not the only writer: a PUT, an `override_columns` sync and an + // import all reach the columns directly, so a dataset can arrive already + // violating the invariant. The mapped column's own transform stays. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { + column_name: 'other_time', + partition_value_transform: 'to_unixtime(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const cleared = clearUnmappedTransforms(MAPPED_TO_EVENT_TIME, columns); + + expect(cleared[0]).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }); + expect(cleared[1]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('with no mapping at all, no column may hold a transform', () => { + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + }, + ]; + + expect( + clearUnmappedTransforms({ main_dttm_col: 'event_time' }, columns)[0] + .partition_value_transform, + ).toBeNull(); +}); + +test('a sync that re-points the mapping cannot arm what the new column holds', () => { + // NEW-R12-01. The mapping sits explicitly on `country`; `event_time` is the + // default datetime column and is holding a transform some earlier writer + // stranded there. A column sync drops `country`, so the override is repaired + // to null and the mapping falls back to `event_time`. + // + // Asked only about the mapping the repair leaves behind, the invariant reads + // `event_time` as the mapped column and keeps its transform -- which then + // goes live from a sync nobody typed into: 181 rows instead of 183, 0 instead + // of 1 for an equality, and the pruning indicator still green. It has to be + // asked on both sides of the move. + const before = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + partition_mapped_column: 'country', + }; + const after = { ...before, partition_mapped_column: null }; + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + // The bug, stated as the single-sided answer this replaced. + expect(clearUnmappedTransforms(after, columns)[0]).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + }); + + expect( + clearUnmappedTransformsAcrossMove(before, after, columns)[0], + ).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('a sync that moves nothing leaves the mapped transform alone', () => { + // The limit of the rule above: with both sides agreeing on the mapped column + // there is no move, and a sync must not cost the owner the transform they + // wrote. Only a transform parked elsewhere goes. + const datasource = { + main_dttm_col: 'event_time', + partition_column: 'dt_epoch', + partition_mapped_column: null, + }; + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { + column_name: 'other_time', + partition_value_transform: 'to_unixtime(:value)', + }, + ]; + + const kept = clearUnmappedTransformsAcrossMove( + datasource, + datasource, + columns, + ); + + expect(kept[0]).toMatchObject({ + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }); + expect(kept[1].partition_value_transform).toBeNull(); +}); + +test('clearing strays when there are none hands back the same columns', () => { + // Same contract as `clearMappingTransforms`, and for the same reason: the + // editor's validation pass keys off column state's identity. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + }, + { column_name: 'country' }, + ]; + + expect(clearUnmappedTransforms(MAPPED_TO_EVENT_TIME, columns)).toBe(columns); +}); + +test('the value transform does not follow the default datetime column', () => { + // A transform states how *this* column relates to the partition column, so + // re-asserting it on a different one turns mirroring on with an expression + // nobody wrote for it -- and the rows it prunes are silently wrong. + const columns = [ + { + column_name: 'event_time', + is_dttm: true, + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + { column_name: 'event_time2', is_dttm: true }, + ]; + + const moved = applyImplicitMappingMove(columns, 'event_time', 'event_time2'); + + expect(moved[0]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); + // The destination never held a mapping, so it is handed back untouched -- + // which is the same thing as holding no transform. + expect(moved[1].partition_value_transform ?? null).toBeNull(); + expect(moved[1].partition_transform_is_monotonic ?? false).toBe(false); +}); + +test('following the default datetime column leaves no transform behind anywhere', () => { + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + }, + { column_name: 'event_time2' }, + { + column_name: 'legacy_time', + partition_value_transform: 'to_unixtime(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyImplicitMappingMove(columns, 'event_time', 'event_time2'); + + expect(moved[2]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('moving the default datetime column onto a column this list has no row for clears too', () => { + // A calculated column can be the default datetime column but never renders a + // transform editor, so a transform left live there is one the owner has no + // way to see or undo. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyImplicitMappingMove(columns, 'event_time', 'calc_time'); + + expect(moved[0]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('moving the default datetime column when nothing holds a mapping is not a change', () => { + // `clearMappingTransforms` hands back the same array when no column held + // anything, and the editor's validation effect keys off column identity. + const columns = [ + { column_name: 'event_time' }, + { column_name: 'event_time2' }, + ]; + + expect(applyImplicitMappingMove(columns, 'event_time', 'event_time2')).toBe( + columns, + ); +}); + +test('re-selecting the same default datetime column leaves the mapping alone', () => { + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + }, + ]; + + expect(applyImplicitMappingMove(columns, 'event_time', 'event_time')).toBe( + columns, + ); +}); + +test('clearing the default datetime column clears the mapping transform with it', () => { + // With no default datetime column and no override there is no mapped column, + // so the transform has nowhere to live and must not wait for one. + const columns = [ + { + column_name: 'event_time', + partition_value_transform: 'unix_timestamp(:value)', + partition_transform_is_monotonic: true, + }, + ]; + + const moved = applyImplicitMappingMove(columns, 'event_time', undefined); + + expect(moved[0]).toMatchObject({ + partition_value_transform: null, + partition_transform_is_monotonic: false, + }); +}); + +test('only temporal columns get a pre-filled transform', () => { + const datasource = { + partition_value_transform_default: 'unix_timestamp(:value)', + }; + + expect(defaultTransformFor(datasource, COLUMNS[0])).toBe( + 'unix_timestamp(:value)', + ); + expect(defaultTransformFor(datasource, COLUMNS[2])).toBe(''); +}); + +test('a temporal column stored as a number or as text gets no pre-fill', () => { + // The engine default takes a timestamp. On an epoch `BIGINT` both Hive and + // Impala reject `unix_timestamp(:value)`, so the pre-fill left the mapping + // broken until the owner replaced it. + const datasource = { + partition_value_transform_default: 'unix_timestamp(:value)', + }; + expect( + defaultTransformFor(datasource, { + column_name: 'event_time', + type: 'BIGINT', + is_dttm: true, + python_date_format: 'epoch_s', + }), + ).toBe(''); + expect( + defaultTransformFor(datasource, { + column_name: 'ds', + type: 'STRING', + is_dttm: true, + python_date_format: '%Y%m%d', + }), + ).toBe(''); + expect( + defaultTransformFor(datasource, { + column_name: 'event_time', + type: 'DATETIME', + is_dttm: true, + }), + ).toBe('unix_timestamp(:value)'); + // An undeclared type keeps the default, as before. + expect( + defaultTransformFor(datasource, { + column_name: 'event_time', + is_dttm: true, + }), + ).toBe('unix_timestamp(:value)'); +}); + +test('a mapping onto an array partition column is never active', () => { + // SC-128852: the banner said filters would apply to `region_keys TEXT[]`, Review Comment: Nit: Remove the internal ticket reference from this public test comment. ```suggestion // The banner said filters would apply to `region_keys TEXT[]`, ``` ########## tests/unit_tests/connectors/sqla/partition_mapping_test.py: ########## @@ -0,0 +1,3801 @@ +# 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. +"""Unit tests for ``superset.connectors.sqla.partition_mapping``.""" + +from __future__ import annotations + +from datetime import datetime +from decimal import Decimal +from importlib import import_module +from typing import Any, cast +from unittest.mock import MagicMock, patch, PropertyMock + +import pandas as pd +import pytest +from dateutil.relativedelta import relativedelta +from flask import Flask +from pytest_mock import MockerFixture +from sqlalchemy.dialects import mysql, postgresql + +from superset.config import DISALLOWED_SQL_FUNCTIONS +from superset.connectors.sqla.models import SqlaTable, TableColumn +from superset.connectors.sqla.partition_mapping import ( + _mirror_verdict_cache_key, + _placeholder_is_bindable, + _probe_cache_key, + _reads_as_temporal, + _render_literal, + _render_predicate, + BARE_WALL_CLOCK_KEYWORDS, + build_mirrored_predicates, + build_probe_sql, + contains_executable_comment, + contains_jinja, + contains_value_placeholder, + declares_array_type, + drop_unmapped_value_transforms, + equality_mirrors_safely, + evaluate_transform, + find_non_deterministic_functions, + grain_bucket_width, + GRAIN_BUCKET_WIDTHS, + is_bare_expression, + is_parseable, + is_transform_active, + is_unfinished, + known_mirror_verdict, + literal_round_trips, + MappingValidationIssue, + mirror_operator, + MIRRORABLE_ALWAYS, + MIRRORABLE_IF_MONOTONIC, + mirrorable_operators, + NON_DETERMINISTIC_FUNCTIONS, + parse_error_detail, + placeholder_is_executable, + PROBE_CACHE_VERSION, + probe_sql_is_evaluable, + probed_value_type_error, + RawProbeValue, + resolve_partition_mapping, + stored_expression_error, + validate_partition_mapping, + validate_transform, +) +from superset.constants import TimeGrain +from superset.db_engine_specs.base import BaseEngineSpec +from superset.db_engine_specs.mysql import MySQLEngineSpec +from superset.db_engine_specs.oracle import OracleEngineSpec +from superset.db_engine_specs.postgres import PostgresEngineSpec +from superset.models.core import Database +from superset.sql.parse import SQLStatement +from superset.utils.core import FilterOperator, GenericDataType, override_user + + [email protected](autouse=True) +def real_probe_cache(app: Flask) -> Any: + """ + The test app runs a null cache, which would make every cache assertion here + vacuously pass. Swap in a real in-memory one for the duration. + """ + from flask_caching import Cache + + from superset.extensions import cache_manager + + cache = Cache(config={"CACHE_TYPE": "SimpleCache", "CACHE_DEFAULT_TIMEOUT": 300}) + cache.init_app(app) + original = cache_manager._cache # noqa: SLF001 + cache_manager._cache = cache # noqa: SLF001 + yield + cache_manager._cache = original # noqa: SLF001 + + [email protected](autouse=True) +def enable_partition_filter_mapping(app: Flask) -> Any: + """The feature ships off; every test here exercises it on.""" + original = app.config["DEFAULT_FEATURE_FLAGS"].get("PARTITION_FILTER_MAPPING") + app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = True + yield + if original is None: + del app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] + else: + app.config["DEFAULT_FEATURE_FLAGS"]["PARTITION_FILTER_MAPPING"] = original + + +def _table(**kwargs: Any) -> SqlaTable: + database = Database(database_name="test_db", sqlalchemy_uri="sqlite://") + defaults: dict[str, Any] = { + "table_name": "web_events", + "database": database, + "main_dttm_col": "event_time", + "columns": [ + TableColumn(column_name="event_time", is_dttm=True, type="TIMESTAMP"), + TableColumn( + column_name="dt_epoch", + type="BIGINT", + partition_value_transform=None, + ), + TableColumn(column_name="country", type="VARCHAR"), + TableColumn(column_name="region_key", type="VARCHAR"), + ], + } + defaults.update(kwargs) + return SqlaTable(**defaults) + + +def _mapped_table( + *, + partition_column: str = "dt_epoch", + mapped_column: str = "event_time", + transform: str | None = "unix_timestamp(:value)", + monotonic: bool = True, + partition_mapped_column: str | None = None, + main_dttm_col: str | None = "event_time", +) -> SqlaTable: + table = _table(main_dttm_col=main_dttm_col) + table.partition_column = partition_column + table.partition_mapped_column = partition_mapped_column + for column in table.columns: + if column.column_name == mapped_column: + column.partition_value_transform = transform + column.partition_transform_is_monotonic = monotonic + return table + + +# --------------------------------------------------------------------------- +# §2 — operator safety matrix +# --------------------------------------------------------------------------- + + +def test_equality_and_in_are_always_mirrorable() -> None: + """``=`` and ``IN`` are safe for any function ``T``.""" + assert MIRRORABLE_ALWAYS == {FilterOperator.EQUALS, FilterOperator.IN} + + +def test_range_operators_require_a_monotonic_transform() -> None: + assert MIRRORABLE_IF_MONOTONIC == { + FilterOperator.GREATER_THAN, + FilterOperator.GREATER_THAN_OR_EQUALS, + FilterOperator.LESS_THAN, + FilterOperator.LESS_THAN_OR_EQUALS, + FilterOperator.TEMPORAL_RANGE, + } + + +def test_mirrorable_operators_excludes_ranges_when_not_monotonic() -> None: + assert mirrorable_operators(is_monotonic=False) == MIRRORABLE_ALWAYS + + +def test_mirrorable_operators_includes_ranges_when_monotonic() -> None: + assert mirrorable_operators(is_monotonic=True) == ( + MIRRORABLE_ALWAYS | MIRRORABLE_IF_MONOTONIC + ) + + [email protected]( + "operator", + [ + FilterOperator.NOT_EQUALS, + FilterOperator.NOT_IN, + FilterOperator.LIKE, + FilterOperator.ILIKE, + FilterOperator.NOT_LIKE, + FilterOperator.NOT_ILIKE, + FilterOperator.IS_NULL, + FilterOperator.IS_NOT_NULL, + FilterOperator.IS_TRUE, + FilterOperator.IS_FALSE, + ], +) +def test_negations_and_pattern_matches_are_never_mirrorable( + operator: FilterOperator, +) -> None: + """ + ``T`` is not injective, so ``col != v`` does **not** imply ``T(col) != T(v)``: + mirroring it would drop rows the original filter keeps. + """ + assert operator not in mirrorable_operators(is_monotonic=True) + + +def test_equality_is_dropped_when_the_engine_does_not_compare_byte_exactly() -> None: + """ + "Safe for any function ``T``" reasons about value equality: ``T`` is a + function, so ``col = v`` gives ``T(col) = T(v)``. The engine reasons about + *SQL* equality, and the two part company under a case-insensitive + collation -- a stored ``'us'`` satisfies a filter for ``'US'`` while the + mirror ``hex('US')`` excludes the row, and the chart loses it silently. + """ + assert mirrorable_operators(is_monotonic=False, equality_is_safe=False) == set() + + +def test_ranges_survive_an_unsafe_equality() -> None: + """ + Range mirroring already rests on the owner declaring ``T`` + order-preserving with respect to the column's own order -- an assertion + about the very comparison semantics this gate checks for. Equality has no + such declaration behind it, which is why only equality is withdrawn. + """ + assert ( + mirrorable_operators(is_monotonic=True, equality_is_safe=False) + == MIRRORABLE_IF_MONOTONIC + ) + + [email protected]( + "operator,expected", + [ + (FilterOperator.GREATER_THAN, FilterOperator.GREATER_THAN_OR_EQUALS), + (FilterOperator.LESS_THAN, FilterOperator.LESS_THAN_OR_EQUALS), + ( + FilterOperator.GREATER_THAN_OR_EQUALS, + FilterOperator.GREATER_THAN_OR_EQUALS, + ), + (FilterOperator.LESS_THAN_OR_EQUALS, FilterOperator.LESS_THAN_OR_EQUALS), + (FilterOperator.EQUALS, FilterOperator.EQUALS), + (FilterOperator.IN, FilterOperator.IN), + ], +) +def test_a_strict_bound_mirrors_non_strictly( + operator: FilterOperator, expected: FilterOperator +) -> None: + """ + Order-preserving means non-decreasing, not injective, so ``col < v`` + implies only ``T(col) <= T(v)``. A day-key transform maps a whole day onto + one value and a strict mirror drops the boundary bucket. The non-strict + cases are already safe and pass through, which is what makes applying this + twice harmless. + """ + assert mirror_operator(operator) == expected + + +def test_relaxing_a_bound_stays_inside_the_mirrorable_set() -> None: + """ + The substitution is about strictness, not about which filters mirror. The + Explore indicator reads `mirrorable_operators`, so leaving that set alone is + what keeps the glyph from promising pruning the SQL does not do -- and it is + also what guarantees `handle_comparison_filter` is never handed an operator + it has no case for. + """ + assert { + mirror_operator(operator) for operator in MIRRORABLE_IF_MONOTONIC + } <= MIRRORABLE_IF_MONOTONIC + + +def _column(type_: str) -> TableColumn: + database = Database(database_name="t", sqlalchemy_uri="sqlite://") + table = SqlaTable(table_name="t", database=database) + return TableColumn(column_name="c", type=type_, table=table) + + [email protected]( + "type_, binary, expected", + [ + ("VARCHAR", True, True), + ("VARCHAR", False, False), + # Numeric and temporal comparison is exact everywhere. + ("BIGINT", False, True), + ("TIMESTAMP", False, True), + ("DOUBLE", False, True), + ], +) +def test_equality_mirrors_safely_only_guards_string_columns( + app: Flask, type_: str, binary: bool, expected: bool +) -> None: + class _Spec(BaseEngineSpec): + binary_string_comparison = binary + + with app.app_context(): + assert equality_mirrors_safely(_column(type_), _Spec) is expected + + [email protected]( + "type_, expected", + [ + # The padded types: `'US'` and `'US '` are equal in the column, and + # `lower()` of the two is not. + ("CHAR(2)", False), + ("CHAR", False), + ("CHARACTER(2)", False), + ("NCHAR(2)", False), + # What PostgreSQL reflection reports for a `CHAR(n)` column. + ("BPCHAR", False), + ("bpchar(2)", False), + ("NATIONAL CHARACTER(2)", False), + # The variable-width types start with the same letters and must not be + # caught by it. + ("VARCHAR(2)", True), + ("NVARCHAR(2)", True), + ("CHARACTER VARYING(2)", True), + ("TEXT", True), + ], +) +def test_a_padded_character_column_declines_equality_on_a_byte_exact_engine( + app: Flask, type_: str, expected: bool +) -> None: + """ + `binary_string_comparison` speaks for the engine's default comparison, + which PostgreSQL declares -- and a `CHAR(2)` there still matches a filter + for `'US '` against a stored `'US'`, so the mirror `lower('US ')` excludes + the row. Padding is a property of the type, so it is asked per column. + """ + + class _Spec(BaseEngineSpec): + binary_string_comparison = True + + with app.app_context(): + assert equality_mirrors_safely(_column(type_), _Spec) is expected + + +def test_redshift_does_not_opt_in_to_byte_exact_comparison() -> None: + """ + Redshift ignores trailing blanks for `VARCHAR` as well as `CHAR`, so the + `True` it inherits from `PostgresBaseEngineSpec` would be wrong. + """ + from superset.db_engine_specs.redshift import RedshiftEngineSpec + + assert RedshiftEngineSpec.binary_string_comparison is False + + [email protected]( + "type_", + [ + # PostgreSQL's case-insensitive text type matches no type pattern, so + # the spec cannot classify it -- and its whole purpose is the + # comparison this gate is about. + "citext", + # A column with no declared type at all. + "", + "SOME_EXTENSION_TYPE", + ], +) +def test_equality_mirrors_safely_fails_closed_on_an_unresolvable_type( + app: Flask, type_: str +) -> None: + """ + The gate exists to stop a silent wrong answer, so a column whose type it + cannot read is treated as the risky case rather than waved through. + + `type_generic` answers `None` for these -- neither string nor not-string -- + and the old reading waved them through, which for `citext` admitted exactly + the case-insensitive equality the gate is for. Only the exception branch + failed closed, though the docstring claimed otherwise. + """ + with app.app_context(): + assert equality_mirrors_safely(_column(type_), PostgresEngineSpec) is False + + +def test_equality_mirrors_safely_reads_past_the_temporal_flag(app: Flask) -> None: + """ + `TableColumn.type_generic` answers `TEMPORAL` for anything carrying + `is_dttm`, so a `VARCHAR` flagged temporal skipped the string check + entirely: on MySQL a filter for `'2026-01-01T00:00:00'` matches a stored + `'...t00:00:00'` while the mirror `hex('...T...')` excludes it. + """ + with app.app_context(): + flagged = _column("VARCHAR") + flagged.is_dttm = True + assert flagged.type_generic == GenericDataType.TEMPORAL + # Declines where plain text declines... + assert equality_mirrors_safely(flagged, MySQLEngineSpec) is False + # ...and is admitted where plain text is admitted. + assert equality_mirrors_safely(flagged, PostgresEngineSpec) is True + + # A column that really is temporal is unaffected. + real = _column("TIMESTAMP") + real.is_dttm = True + assert equality_mirrors_safely(real, MySQLEngineSpec) is True + + +def test_no_mapped_column_leaves_the_matrix_alone(app: Flask) -> None: + """Nothing to reason about, and the resolver bails out on it anyway.""" + with app.app_context(): + assert equality_mirrors_safely(None, BaseEngineSpec) is True + + [email protected]( + "module, spec_name", + [ + ("presto", "PrestoEngineSpec"), + ("hive", "HiveEngineSpec"), + ("trino", "TrinoEngineSpec"), + ("impala", "ImpalaEngineSpec"), + ("postgres", "PostgresEngineSpec"), + ("bigquery", "BigQueryEngineSpec"), + ("sqlite", "SqliteEngineSpec"), + ], +) +def test_the_byte_exact_engines_opt_in(module: str, spec_name: str) -> None: + """ + The engines this feature targets compare text byte-exactly, so the gate + must not cost them the second canonical mapping (`lower(:value)` onto a + lowercased key). + """ + spec = getattr(import_module(f"superset.db_engine_specs.{module}"), spec_name) + assert spec.binary_string_comparison is True + + [email protected]("module, spec_name", [("mysql", "MySQLEngineSpec")]) +def test_a_case_insensitive_engine_does_not_opt_in(module: str, spec_name: str) -> None: + """ + MySQL's default collation is `utf8mb4_0900_ai_ci`, which is exactly the + case the reviewer raised. + """ + spec = getattr(import_module(f"superset.db_engine_specs.{module}"), spec_name) + assert spec.binary_string_comparison is False + + +def test_the_base_spec_does_not_opt_in() -> None: + """A spec that has not said so does not mirror string equality.""" + assert BaseEngineSpec.binary_string_comparison is False + + +# --------------------------------------------------------------------------- +# Time grain bucket widths +# --------------------------------------------------------------------------- + + [email protected]("grain", list(TimeGrain)) +def test_every_builtin_grain_has_a_known_bucket_width( + app: Flask, grain: TimeGrain +) -> None: + """ + The widening is only sound for a grain whose bucket width we know, so a + grain added upstream without a width here would silently stop mirroring. + This is the test that notices. + """ + with app.app_context(): + assert grain_bucket_width(grain.value, "sqlite") is not None + + +def test_a_calendar_grain_widens_by_a_calendar_month(app: Flask) -> None: + """A month is not thirty days, and the month it lands in decides its width.""" + with app.app_context(): + width = grain_bucket_width(TimeGrain.MONTH.value, "sqlite") + + assert width is not None + assert datetime(2026, 1, 31) + width == datetime(2026, 2, 28) + assert datetime(2026, 2, 1) + width == datetime(2026, 3, 1) + + +def test_a_fractional_grain_has_a_width(app: Flask) -> None: + """ + ``PT0.5H`` and ``P0.25Y`` are why the widths are written out rather than + parsed -- and the anchored week grains are intervals ``parse_duration`` + rejects outright. + """ + with app.app_context(): + assert grain_bucket_width(TimeGrain.HALF_HOUR.value, "sqlite") == relativedelta( + minutes=30 + ) + assert grain_bucket_width( + TimeGrain.WEEK_ENDING_SATURDAY.value, "sqlite" + ) == relativedelta(days=7) + + +def test_an_unknown_grain_has_no_known_width(app: Flask) -> None: + with app.app_context(): + assert grain_bucket_width("P13X", "sqlite") is None + assert grain_bucket_width(None, "sqlite") is None + + +def test_an_addon_grain_has_no_known_width(app: Flask) -> None: + """An operator-declared grain carries whatever duration string they typed.""" + with app.app_context(): + app.config["TIME_GRAIN_ADDONS"] = {"PT2S": "2 second"} + try: + assert grain_bucket_width("PT2S", "sqlite") is None + finally: + app.config["TIME_GRAIN_ADDONS"] = {} + + +def test_a_builtin_grain_an_operator_redefined_has_no_known_width( + app: Flask, +) -> None: + """ + ``TIME_GRAIN_ADDON_EXPRESSIONS`` replaces a built-in grain's SQL for one + engine, so its duration no longer bounds the displacement *there* -- but it + still does on every other engine. + """ + with app.app_context(): + app.config["TIME_GRAIN_ADDON_EXPRESSIONS"] = { + "sqlite": {"P1D": "DATETIME({col}, 'start of year')"} + } + try: + assert grain_bucket_width(TimeGrain.DAY.value, "sqlite") is None + assert grain_bucket_width(TimeGrain.DAY.value, "hive") is not None + finally: + app.config["TIME_GRAIN_ADDON_EXPRESSIONS"] = {} + + +def test_the_width_table_is_keyed_on_grain_durations(app: Flask) -> None: + """Keys are the ISO duration a filter carries, not the enum member name.""" + assert GRAIN_BUCKET_WIDTHS[TimeGrain.DAY] == relativedelta(days=1) + assert set(GRAIN_BUCKET_WIDTHS) == {grain.value for grain in TimeGrain} + + +# --------------------------------------------------------------------------- +# §4.1 — resolving the effective mapping, and the defensive bail-outs +# --------------------------------------------------------------------------- + + +def test_resolve_returns_the_mapping_when_everything_lines_up(app: Flask) -> None: + with app.app_context(): + mapping = resolve_partition_mapping(_mapped_table()) + + assert mapping is not None + assert mapping.partition_column == "dt_epoch" + assert mapping.mapped_column == "event_time" + assert mapping.value_transform == "unix_timestamp(:value)" + assert mapping.is_monotonic is True + + +def test_effective_mapped_column_follows_main_dttm_col(app: Flask) -> None: + """No explicit override: the mapping follows the default datetime column.""" + with app.app_context(): + mapping = resolve_partition_mapping(_mapped_table()) + + assert mapping is not None + assert mapping.mapped_column == "event_time" + + +def test_explicit_override_wins_over_main_dttm_col(app: Flask) -> None: + table = _mapped_table( + mapped_column="country", + transform="lower(:value)", + monotonic=False, + partition_mapped_column="country", + ) + table.partition_column = "region_key" + + with app.app_context(): + mapping = resolve_partition_mapping(table) + + assert mapping is not None + assert mapping.mapped_column == "country" + assert mapping.partition_column == "region_key" + assert mapping.is_monotonic is False + + +def test_resolve_returns_none_when_the_feature_flag_is_off(app: Flask) -> None: + table = _mapped_table() + with app.app_context(): + with patch( + "superset.connectors.sqla.partition_mapping.feature_flag_manager." + "is_feature_enabled", + return_value=False, + ): + assert resolve_partition_mapping(table) is None + + +def test_an_array_partition_column_resolves_no_mapping(app: Flask) -> None: + """ + SC-128852, page 3: with only the per-value refusal the chart stopped Review Comment: Nit: Remove the internal ticket reference from this public test comment. ```suggestion With only the per-value refusal the chart stopped ``` -- 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]
