codeant-ai-for-open-source[bot] commented on code in PR #41279:
URL: https://github.com/apache/superset/pull/41279#discussion_r3721059924
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx:
##########
@@ -127,6 +130,17 @@ export const useSimpleTabFilterProps = (props: Props) => {
// hide the TEMPORAL_RANGE operator
return false;
}
+ // CONTAINS (array membership) only applies to multi-value columns.
+ if (operator === Operators.Contains) {
+ return isColumnMultiValue;
+ }
+ if (isColumnMultiValue) {
+ // multi-value columns support membership and null checks only;
+ // CONTAINS is already handled by the early return above.
+ return (
+ operator === Operators.IsNull || operator === Operators.IsNotNull
+ );
Review Comment:
**Suggestion:** Returning false for all operators except null checks and
`CONTAINS` makes an existing operator irrelevant when the subject changes to a
multi-value column, but `onSubjectChange` then falls back to `IN`. The UI
consequently stores an `IN` filter even though `IN` is hidden for that subject;
submitting or reopening the filter can produce an invalid/non-membership array
predicate instead of requiring the user to choose `CONTAINS` or a null check.
Ensure the fallback operator is valid for multi-value subjects, or clear the
operator until one is selected. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Array filters retain hidden scalar IN operators.
- ❌ Queries can use invalid array membership semantics.
- ⚠️ Reopening filters shows inconsistent operator state.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=649a5e78871a412fbd97d7d263b95d05&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=649a5e78871a412fbd97d7d263b95d05&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-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx
**Line:** 137:142
**Comment:**
*Api Mismatch: Returning false for all operators except null checks and
`CONTAINS` makes an existing operator irrelevant when the subject changes to a
multi-value column, but `onSubjectChange` then falls back to `IN`. The UI
consequently stores an `IN` filter even though `IN` is hidden for that subject;
submitting or reopening the filter can produce an invalid/non-membership array
predicate instead of requiring the user to choose `CONTAINS` or a null check.
Ensure the fallback operator is valid for multi-value subjects, or clear the
operator until one is selected.
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=aa21c6ea42215a6243f75bb863bd010597b5fb642d0eedda1eddc010a1f7188f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41279&comment_hash=aa21c6ea42215a6243f75bb863bd010597b5fb642d0eedda1eddc010a1f7188f&reaction=dislike'>👎</a>
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -134,7 +136,7 @@ def is_read_limit_error(cls, ex: Exception) -> bool:
(
re.compile(r".*Array.*", re.IGNORECASE),
types.String(),
- GenericDataType.STRING,
+ GenericDataType.MULTI_VALUE,
Review Comment:
**Suggestion:** Mapping every ClickHouse `Array(...)` column to SQLAlchemy
`String` loses the element type needed by the new `CONTAINS` operation. The
filter coercion path does not convert values for `GenericDataType.MULTI_VALUE`,
so a UI value such as `1` for `Array(UInt8)` remains a string bind value and
`has(array_column, '1')` can fail ClickHouse's argument-type check. Preserve or
derive the array element type, or explicitly coerce the contains value using
the array's element type. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Contains filters fail for numeric ClickHouse arrays.
- ⚠️ Numeric array membership receives string binds.
- ⚠️ Array support works only reliably for string elements.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=83174935b6de474f99db662109eb8c23&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=83174935b6de474f99db662109eb8c23&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/db_engine_specs/clickhouse.py
**Line:** 137:139
**Comment:**
*Type Error: Mapping every ClickHouse `Array(...)` column to SQLAlchemy
`String` loses the element type needed by the new `CONTAINS` operation. The
filter coercion path does not convert values for `GenericDataType.MULTI_VALUE`,
so a UI value such as `1` for `Array(UInt8)` remains a string bind value and
`has(array_column, '1')` can fail ClickHouse's argument-type check. Preserve or
derive the array element type, or explicitly coerce the contains value using
the array's element type.
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=69640b98d88101d7894b3a21eac1ace94ed7da82260ae1b9e97c0a4381130e1d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41279&comment_hash=69640b98d88101d7894b3a21eac1ace94ed7da82260ae1b9e97c0a4381130e1d&reaction=dislike'>👎</a>
##########
superset/connectors/sqla/models.py:
##########
@@ -1789,6 +1789,63 @@ def _render_adhoc_expression_for_metadata_lookup(
)
) from ex
+ def _multivalue_column_to_sqla(
+ self,
+ col: AdhocColumn,
+ template_processor: BaseTemplateProcessor | None = None,
+ ) -> tuple[ColumnElement, utils.GenericDataType | None]:
+ """
+ Turn a multi-value (array) modifier column into a sqlalchemy column.
+
+ The column references a base array column and an operation (e.g. array
+ length); the native SQL is produced by the engine spec so the same
+ payload works across any dialect that supports array columns.
+ """
+ label = utils.get_column_name(col)
+ base_name = col.get("column")
+ operation = col.get("columnOperation")
+
+ db_engine_spec = self.db_engine_spec
+ if not db_engine_spec.supports_multivalue_columns:
+ raise QueryObjectValidationError(
+ _("This database does not support multi-value (array)
columns.")
+ )
+
+ base_column = self.get_column(base_name)
+ if base_column is None:
+ raise QueryObjectValidationError(
+ _("Unknown column used as multi-value source: %(col)s",
col=base_name)
+ )
+ base_sqla_col =
base_column.get_sqla_col(template_processor=template_processor)
Review Comment:
**Suggestion:** The modifier path verifies only that the engine advertises
multi-value support and that the source name exists; it never verifies that
`base_column` is actually `GenericDataType.MULTI_VALUE`. A manually submitted
LENGTH or EXPLODE modifier can therefore target a scalar column and emit
`length(scalar)` or `arrayJoin(scalar)`, resulting in invalid ClickHouse SQL at
execution time. Reject the modifier unless the source column's metadata
identifies it as an array column. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Crafted modifier queries fail during ClickHouse execution.
- ⚠️ Length and explode dimensions lack backend type validation.
- ⚠️ Explore requests return database errors instead of validation errors.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62b32c98e61b4b898fc9400bfa9f3819&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=62b32c98e61b4b898fc9400bfa9f3819&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/connectors/sqla/models.py
**Line:** 1814:1819
**Comment:**
*Type Error: The modifier path verifies only that the engine advertises
multi-value support and that the source name exists; it never verifies that
`base_column` is actually `GenericDataType.MULTI_VALUE`. A manually submitted
LENGTH or EXPLODE modifier can therefore target a scalar column and emit
`length(scalar)` or `arrayJoin(scalar)`, resulting in invalid ClickHouse SQL at
execution time. Reject the modifier unless the source column's metadata
identifies it as an array column.
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=3c76cc59eaae48cb24309ca061b1d0e90f5e8d323c823bf3a04065aa834bf9a4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41279&comment_hash=3c76cc59eaae48cb24309ca061b1d0e90f5e8d323c823bf3a04065aa834bf9a4&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -4230,6 +4237,17 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
target_clause_list.append(sqla_col.not_like(eq))
else:
target_clause_list.append(sqla_col.not_ilike(eq))
+ elif op == utils.FilterOperator.CONTAINS:
+ if not db_engine_spec.supports_multivalue_columns:
+ raise QueryObjectValidationError(
+ _(
+ "The CONTAINS operator is only supported
for "
+ "multi-value (array) columns on this
database."
+ )
+ )
+ target_clause_list.append(
+ db_engine_spec.array_contains(sqla_col, eq)
+ )
Review Comment:
**Suggestion:** The capability check only verifies that the database
supports array operations, not that the resolved filter target is actually a
`MULTI_VALUE` column. API clients or persisted query payloads can submit
`CONTAINS` for a scalar column on ClickHouse, causing `array_contains` to
generate `has(scalar_column, value)` and fail at query execution instead of
returning a validation error. Reject the operator unless `target_generic_type`
is `GenericDataType.MULTI_VALUE`. [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Scalar CONTAINS queries generate invalid ClickHouse SQL.
- ⚠️ API clients receive execution errors instead of validation errors.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0b1f307ffa6d48b8a7ac36c9a394ac11&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=0b1f307ffa6d48b8a7ac36c9a394ac11&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/models/helpers.py
**Line:** 4240:4250
**Comment:**
*Api Mismatch: The capability check only verifies that the database
supports array operations, not that the resolved filter target is actually a
`MULTI_VALUE` column. API clients or persisted query payloads can submit
`CONTAINS` for a scalar column on ClickHouse, causing `array_contains` to
generate `has(scalar_column, value)` and fail at query execution instead of
returning a validation error. Reject the operator unless `target_generic_type`
is `GenericDataType.MULTI_VALUE`.
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=13cb76e994f1e433c04ba204ca5bfb05ad330606bd634833d2700ea8b55716cf&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41279&comment_hash=13cb76e994f1e433c04ba204ca5bfb05ad330606bd634833d2700ea8b55716cf&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]
