gabotorresruiz commented on code in PR #44999:
URL: https://github.com/apache/superset/pull/44999#discussion_r4276536618
##########
superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts:
##########
@@ -94,12 +94,27 @@ export const shouldShowTimeRangePicker = (
currentDataset: (Dataset & { column_types: GenericDataType[] }) | undefined,
): boolean => (currentDataset ? hasTemporalColumns(currentDataset) : true);
-export const doesColumnMatchFilterType = (filterType: string, column: Column)
=>
- !column.type_generic ||
- !(filterType in FILTER_SUPPORTED_TYPES) ||
- FILTER_SUPPORTED_TYPES[
- filterType as keyof typeof FILTER_SUPPORTED_TYPES
- ]?.includes(column.type_generic);
+export const doesColumnMatchFilterType = (
+ filterType: string,
+ column: Column,
+ datasourceType?: DatasourceType,
+): boolean => {
+ // Semantic range filters skip the runtime non-numeric bounds check, so an
+ // unrecognised dimension type must not be offered for >= / <= comparisons.
+ if (
+ filterType === 'filter_range' &&
+ datasourceType === DatasourceType.SemanticView
+ ) {
+ return column.type_generic === GenericDataType.Numeric;
+ }
Review Comment:
This block worries me a bit. `type_generic` on a semantic dimension is not
provider metadata: it comes from `mapSemanticTypeToGenericDataType` in
`src/utils/semanticViewStructure.ts`, which is a regex over the provider's type
string and returns `undefined` for anything it does not recognise. I ran it
over a spread of type names on this branch. `number`, `int`, `int32`, `int64`,
`decimal`, `decimal(10,2)`, `float`, `float64` and `double` come back
`Numeric`, but `integer`, `bigint`, `smallint`, `tinyint`, `numeric` and `real`
all come back `undefined`.
On `master` those were still offered to `filter_range` via the
`!column.type_generic` escape hatch. With this change they are not, and
`ColumnSelect.tsx:137` calls `resetColumnField()` on a saved selection that no
longer passes `filterValues`, so a filter already pointing at a `bigint`
dimension loses its column when the modal opens. For a provider that reports
SQL type names rather than Arrow ones the picker would just say `No compatible
columns found`.
Are those type strings impossible from the providers we support? If they are
not, the manual path is arguably the safe place to be permissive, because the
thing that made an unknown type risky was the MIN/MAX aggregate and that is
exactly what this PR removes:
```suggestion
if (
filterType === 'filter_range' &&
datasourceType === DatasourceType.SemanticView
) {
return (
!column.type_generic || column.type_generic === GenericDataType.Numeric
);
}
```
Widening the mapper instead would work just as well. Either way a case in
`semanticViewStructure.test.ts` for `bigint` and `integer` would pin whichever
way you decide.
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -262,8 +280,9 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
formData;
// Get the display mode from formData
- const rangeDisplayMode =
- formData?.rangeDisplayMode || RangeDisplayMode.SliderAndInput;
+ const rangeDisplayMode = manualBounds
+ ? RangeDisplayMode.Input
+ : formData?.rangeDisplayMode || RangeDisplayMode.SliderAndInput;
Review Comment:
`getMessageAndStatus` already treats `min === undefined || max ===
undefined` the same as `manualBounds`, but `rangeDisplayMode` does not, and
`renderSlider` now returns `null` in that case. The result is that a SQL
dataset filter saved with Range Type `Slider` whose column is empty or all
`NULL` renders nothing at all: on this branch I get zero sliders and zero
inputs with `Leave a bound empty for an unbounded range.` underneath, where
`master` rendered the slider. `Slider and range input` is unaffected because
the inputs survive.
Would folding it into the same condition work?
```suggestion
const rangeDisplayMode =
manualBounds || min === undefined || max === undefined
? RangeDisplayMode.Input
: formData?.rangeDisplayMode || RangeDisplayMode.SliderAndInput;
```
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -331,11 +355,32 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
},
});
},
- [setDataMask],
+ [setDataMask, col, enableSingleExactValue],
);
useEffect(() => {
- if (row?.min === undefined && row?.max === undefined) {
+ if (nonNumericColumn) {
+ return;
+ }
+
+ if (manualBounds) {
+ // Invalid edits keep their draft in the inputs; external saved values
+ // and Clear All are validated without requiring a discovered domain.
+ const value: RangeValue =
+ filterState.validateStatus === 'error'
+ ? inputValue
+ : (filterState.value ?? [null, null]);
+ setInputValue(value);
+ const { isValid, errorMessage } = validateRange(
+ value,
+ undefined,
+ undefined,
+ enableEmptyFilter,
+ enableSingleValue,
+ );
Review Comment:
Not a blocker, but this is the one place where the manual path is stricter
than the slider path, and I think it contradicts the doc paragraph you added.
`validateRange` requires a number whenever `enableSingleValue` is set,
regardless of `enableEmptyFilter`, and this is the only branch that runs it on
an externally cleared value. So on a semantic view with Single value type set
to `Minimum`, `Maximum` or `Exact`, Clear All (which the filter bar stages as
`[null, null]` for `filter_range`) lands on `validateStatus: 'error'` with
`Please provide a valid min or max value` even when `Filter value is required`
is off. Rendering both on this branch: semantic emits `{extraFormData: {},
filterState: {value: null, validateStatus: 'error'}}` and shows the message,
while the same clear on a SQL dataset emits `{value: [null, null]}` with no
error. The two-bound mode clears cleanly, it is only the single-value modes.
The docs say `leave both empty to remove the constraint unless Filter value
is required is enabled`, so either that sentence needs a single-value carve out
or the validation should honour `enableEmptyFilter` here too. A `test.each`
over the three `SingleValueType` values, asserting a clear with
`enableEmptyFilter: false` lands on `extraFormData: {}` and `validateStatus:
undefined`, would pin it.
##########
superset-frontend/src/filters/components/Range/isSemanticRange.ts:
##########
@@ -0,0 +1,32 @@
+/**
+ * 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 { DatasourceKey, DatasourceType } from '@superset-ui/core';
+
+/** Identify numeric filters whose source cannot provide aggregate bounds. */
+export default function isSemanticRange(formData: {
+ datasource?: string;
+ viz_type?: string;
+ vizType?: string;
+}): boolean {
+ return (
+ (formData.viz_type ?? formData.vizType) === 'filter_range' &&
Review Comment:
Just a small NIT on coverage. The `datasource` half of this predicate is
well pinned, but the `filter_range` half is not: I dropped it locally and all
six suites still passed, even though it is the only thing stopping
`FilterValue`'s guard from short-circuiting every other filter type on a
semantic view. With it removed, a `filter_select` on `3__semantic_view` never
calls `requestChartDataResolved` and its options come back empty. One more case
in `FilterValue.test.tsx`, a `filter_select` target with `datasourceType:
DatasourceType.SemanticView` asserting `requestChartDataResolved` **is**
called, would close it.
--
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]
