gabotorresruiz commented on code in PR #44999:
URL: https://github.com/apache/superset/pull/44999#discussion_r4285694255
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -176,6 +177,23 @@ const validateRange = (
max,
});
+ if (values.some(value => value !== null && !Number.isFinite(value))) {
+ return { isValid: false, errorMessage: requiredError };
+ }
+
+ // Allow empty ranges if enableEmptyFilter is false
+ if (!enableEmptyFilter && inputMin === null && inputMax === null) {
Review Comment:
Not a blocker, more a question about scope. Moving the empty value checks
above the single value block changes SQL dataset filters too, since
`handleInputChange` calls `validateRange` on the non manual path as well. On
`02606ff` a `3__table` range filter in `Minimum`, `Maximum` or `Exact` mode
rejected a cleared input with `Please provide a valid min or max value` in all
six combinations of mode and `Filter value is required`. On this head the three
with the requirement off accept the clear and emit `[null, null]`, and the
three with it on still reject. That reads like the right behaviour to me, but
the new cases are all semantic, so nothing pins it. Worth one SQL dataset case
so a later reorder cannot quietly take it back?
##########
superset-frontend/src/utils/semanticViewStructure.ts:
##########
@@ -53,7 +53,7 @@ export const mapSemanticTypeToGenericDataType = (
}
if (
- /(\b(u?int\d*)\b|\bfloat\d*\b|\bdouble\b|\bdecimal\d*\b|\bnumber\b)/.test(
+
/(\b(u?int\d*|integer|bigint|smallint|tinyint|numeric|real)\b|\bfloat\d*\b|\bdouble\b|\bdecimal\d*\b|\bnumber\b)/.test(
Review Comment:
Not a blocker, and I may be missing a provider that does this. `GET
/api/v1/semantic_view/<id>/structure` sends `str(dim.type)` and
`Dimension.type` is typed `pa.DataType`, so the spellings that reach this
mapper are Arrow ones. I booted a stub provider with a `pa.float16()` dimension
and `/structure` came back with `halffloat`, which this pattern misses, so with
the new `filter_range` branch in `doesColumnMatchFilterType` that dimension is
no longer offered and `ColumnSelect` resets a saved selection on it. On
`master` it was still offered. One token, if you want it:
```suggestion
/(\b(u?int\d*|integer|bigint|smallint|tinyint|numeric|real|halffloat)\b|\bfloat\d*\b|\bdouble\b|\bdecimal\d*\b|\bnumber\b)/.test(
```
Worth a case next to the six you just added.
--
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]
