mikebridge commented on code in PR #44999:
URL: https://github.com/apache/superset/pull/44999#discussion_r4276605470
##########
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:
Fixed in
[8a21f73b3c](https://github.com/apache/superset/commit/8a21f73b3c8f3e81eea39ae54f471ba62b38928d).
The shared mapper recognizes `integer`, `bigint`, `smallint`, `tinyint`,
`numeric` and `real` as numeric, so those dimensions remain available to the
range picker. Added regression cases for all six names, uppercase
`BIGINT`/`INTEGER`, and `numeric(10,2)`. Unknown types still use the existing
validation.
##########
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:
Fixed in
[8a21f73b3c](https://github.com/apache/superset/commit/8a21f73b3c8f3e81eea39ae54f471ba62b38928d).
Empty optional values are accepted before single-value validation. Added Clear
All cases for Minimum, Maximum and Exact that assert an empty constraint and no
validation error, plus required-value controls that still reject the clear.
This matches the documented behavior.
##########
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:
Fixed in
[8a21f73b3c](https://github.com/apache/superset/commit/8a21f73b3c8f3e81eea39ae54f471ba62b38928d)
using the shared missing-bounds condition for `rangeDisplayMode`. A saved SQL
Slider filter falls back to manual inputs when its column is empty or all NULL.
Both cases have regressions that also enter a bound and verify the emitted
filter.
##########
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:
Added in
[8a21f73b3c](https://github.com/apache/superset/commit/8a21f73b3c8f3e81eea39ae54f471ba62b38928d).
The semantic-view `filter_select` case asserts that `requestChartDataResolved`
is called with `3__semantic_view` and `filter_select`, then renders its result.
The existing semantic range case still asserts no bounds request. All nine
affected Jest suites pass (206 passed, one existing skip).
--
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]
