rusackas commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3564749726
##########
superset/models/helpers.py:
##########
@@ -2637,21 +2637,50 @@ def filter_values_handler( # pylint:
disable=too-many-arguments # noqa: C901
if values is None:
return None
- def handle_single_value(value: Optional[FilterValue]) ->
Optional[FilterValue]:
- if operator == utils.FilterOperator.TEMPORAL_RANGE:
- return value
+ temporal_comparison_operators = {
+ utils.FilterOperator.EQUALS,
+ utils.FilterOperator.NOT_EQUALS,
+ utils.FilterOperator.IN,
+ utils.FilterOperator.NOT_IN,
+ utils.FilterOperator.GREATER_THAN,
+ utils.FilterOperator.LESS_THAN,
+ utils.FilterOperator.GREATER_THAN_OR_EQUALS,
+ utils.FilterOperator.LESS_THAN_OR_EQUALS,
+ }
+
+ def handle_temporal_value(value: FilterValue) -> FilterValue |
ColumnElement:
if (
- isinstance(value, (float, int))
- and target_generic_type == utils.GenericDataType.TEMPORAL
- and target_native_type is not None
- and db_engine_spec is not None
+ operator not in temporal_comparison_operators
+ or target_generic_type != utils.GenericDataType.TEMPORAL
+ or target_native_type is None
+ or db_engine_spec is None
):
- value = db_engine_spec.convert_dttm(
- target_type=target_native_type,
- dttm=datetime.utcfromtimestamp(value / 1000),
- db_extra=db_extra,
+ return value
+
+ if isinstance(value, (float, int)) and not isinstance(value, bool):
+ epoch_ms: float = value
+ elif isinstance(value, str) and re.fullmatch(r"[+-]?\d+", value):
+ epoch_ms = int(value)
Review Comment:
I don't think this is reachable in practice. Only bare all-digit strings hit
this branch, and the only thing sending those to a temporal `==`/`IN` filter is
the epoch-ms formatters this PR wires up. A real date like `2024-01-01` has
separators and falls through untouched. `master` already treated numeric
temporal values as epoch-ms, this just extends that to their string form. A
magnitude guard would clip valid small/pre-1970 epochs, so I'd rather leave it.
##########
superset-frontend/plugins/plugin-chart-pivot-table/src/PivotTableChart.tsx:
##########
@@ -419,10 +461,14 @@ export default function PivotTableChart(props:
PivotTableProps) {
col,
op: 'IS NULL',
};
+ const formatter =
+ typeof col === 'string' ? dateFormatters[col] : undefined;
return {
col,
op: 'IN',
- val: val as (string | number | boolean)[],
+ val: (val as DataRecordValue[]).map(value =>
+ getCrossFilterValue(value, formatter),
+ ) as (string | number | boolean)[],
};
Review Comment:
Good catch, fixed. Resolving the formatter by `key` now instead of gating on
`typeof col === 'string'`, so adhoc temporal groupbys get coerced same as
physical ones.
--
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]
