Elengar commented on code in PR #40180:
URL: https://github.com/apache/superset/pull/40180#discussion_r3564868472
##########
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: set[utils.FilterOperator] = {
+ 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:
This is the known ambiguity already discussed and accepted in review.
Numeric temporal values were already interpreted as epoch milliseconds before
this PR; the change extends that behavior to numeric-string payloads emitted by
drill/cross-filter interactions. A length or magnitude heuristic is not
reliable because valid JavaScript epoch-millisecond values can be signed and
have different digit lengths. Non-integer strings continue to pass through
unchanged.
##########
superset-frontend/plugins/plugin-chart-table/src/TableChart.tsx:
##########
@@ -108,6 +108,25 @@ interface TableSize {
height: number;
}
+const getCrossFilterValue = (
+ value: DataRecordValue,
+ column: DataColumnMeta | undefined,
+): DataRecordValue => {
+ const input = value instanceof DateWithFormatter ? value.input : value;
+ if (
+ column?.dataType === GenericDataType.Temporal &&
+ typeof input === 'string' &&
+ input.trim() !== '' &&
+ Number.isFinite(Number(input))
+ ) {
+ return Number(input);
+ }
+ if (value instanceof Date) {
+ return value.getTime();
+ }
Review Comment:
This behavior predates this PR: the previous cross-filter mapping already
converted every `Date` with `el instanceof Date ? el.getTime() : el`.
`DateWithFormatter` extends `Date`, so preserving Date-to-epoch-ms conversion
is intentional. This PR only adds numeric-string handling before that existing
fallback; returning the original ISO input here would change established
cross-filter semantics.
--
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]
