codeant-ai-for-open-source[bot] commented on code in PR #41279: URL: https://github.com/apache/superset/pull/41279#discussion_r3721055403
########## tests/unit_tests/models/test_multivalue_explode.py: ########## @@ -0,0 +1,169 @@ +# 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. +"""Explode (group by array elements) for multi-value columns โ ClickHouse MVP. + +Set-returning UNNEST dialects (Postgres/Trino/BigQuery) need CROSS JOIN UNNEST +plumbing and are handled in a later phase; here we cover the scalar ClickHouse +``arrayJoin`` path plus the guard that keeps unimplemented dialects from emitting +invalid SQL. +""" + +from __future__ import annotations + +import pytest +from flask import Flask +from pytest_mock import MockerFixture + +from superset.connectors.sqla.models import SqlaTable, SqlMetric, TableColumn +from superset.db_engine_specs.base import BaseEngineSpec +from superset.exceptions import QueryObjectValidationError +from superset.models.core import Database +from superset.superset_typing import AdhocColumn, Column, QueryObjectDict +from superset.utils.core import MultiValueColumnOperation + + +class _UnnestOnlySpec(BaseEngineSpec): + """Simulates a future array dialect that has not implemented scalar explode.""" + + supports_multivalue_columns = True + # array_explode intentionally left as the base NotImplementedError + + +def _make_dataset(mocker: MockerFixture) -> SqlaTable: + database = Database( + id=1, + database_name="test_db", + sqlalchemy_uri="sqlite://", + ) + columns = [ + TableColumn(column_name="skills", type="Array(String)"), + TableColumn(column_name="city", type="VARCHAR(100)"), + ] + dataset = SqlaTable( + table_name="jobs", + columns=columns, + database=database, + metrics=[SqlMetric(metric_name="count", expression="COUNT(*)")], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.get_guest_rls_filters", + return_value=[], + ) + mocker.patch( + "superset.connectors.sqla.models.security_manager.is_guest_user", + return_value=False, + ) + return dataset + + +def _explode_dimension() -> AdhocColumn: + return { + "label": "skill", + "column": "skills", + "columnOperation": MultiValueColumnOperation.EXPLODE.value, + } + + +def _query_obj(dimension: Column) -> QueryObjectDict: + return { + "granularity": None, + "from_dttm": None, + "to_dttm": None, + "is_timeseries": False, + "groupby": [dimension], + "metrics": ["count"], + "filter": [], + "columns": [], + } + + +def _clickhouse_spec() -> type: + # Imported lazily: clickhouse.py touches app.config at import time, which + # is unavailable at pytest collection once clickhouse-connect is installed. + from superset.db_engine_specs.clickhouse import ClickHouseEngineSpec + + return ClickHouseEngineSpec + + +def _with_spec(mocker: MockerFixture, dataset: SqlaTable, spec: type) -> None: + mocker.patch.object( + SqlaTable, + "db_engine_spec", + new=property(lambda self: spec), + ) + + +def test_explode_dimension_generates_arrayjoin( + mocker: MockerFixture, + app: Flask, +) -> None: + """An explode dimension routes through array_explode -> arrayJoin(skills).""" + dataset = _make_dataset(mocker) + _with_spec(mocker, dataset, _clickhouse_spec()) + with app.test_request_context(): + sql = dataset.get_query_str_extended( + _query_obj(_explode_dimension()), mutate=False + ).sql.lower() + + assert "arrayjoin(skills)" in sql + assert "skill" in sql # the label is applied + + +def test_explode_changes_generated_sql( + mocker: MockerFixture, + app: Flask, +) -> None: + """Exploding a column yields different SQL than grouping by it raw. + + The explode modifier is part of the query object, so a query that explodes + cannot collide with one that does not (distinct cache keys downstream). + """ + dataset = _make_dataset(mocker) + _with_spec(mocker, dataset, _clickhouse_spec()) + with app.test_request_context(): + exploded = dataset.get_query_str_extended( + _query_obj(_explode_dimension()), mutate=False + ).sql + plain = dataset.get_query_str_extended(_query_obj("city"), mutate=False).sql + + assert exploded != plain Review Comment: **Suggestion:** The comparison uses an unrelated `city` dimension, so it does not verify that exploding `skills` differs from grouping the same array column without the modifier. Compare the exploded query with a plain `skills` grouping to ensure the `EXPLODE` operation itself changes the generated SQL and cache key. [code quality] <details> <summary><b>Severity Level:</b> Minor ๐งน</summary> ```mdx - โ ๏ธ Explode regression coverage does not isolate operation behavior. - โ ๏ธ Incorrect implementation could pass this test unnoticed. - โ ๏ธ Cache-key distinction is not directly validated by this comparison. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=f10d71e2111e4e3f97651584b31f287e&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=f10d71e2111e4e3f97651584b31f287e&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/models/test_multivalue_explode.py **Line:** 138:143 **Comment:** *Code Quality: The comparison uses an unrelated `city` dimension, so it does not verify that exploding `skills` differs from grouping the same array column without the modifier. Compare the exploded query with a plain `skills` grouping to ensure the `EXPLODE` operation itself changes the generated SQL and cache key. 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%2F41279&comment_hash=f8fdd3ae8b2416bce2c57d06f93028334fd3c7d5d107a17616af73bca0722cb3&reaction=like'>๐</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41279&comment_hash=f8fdd3ae8b2416bce2c57d06f93028334fd3c7d5d107a17616af73bca0722cb3&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: [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]
