hughhhh commented on code in PR #43870:
URL: https://github.com/apache/superset/pull/43870#discussion_r4319674560
##########
superset/models/helpers.py:
##########
@@ -5317,6 +5633,43 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
db_extra=self.db_extra,
)
+ # Mirror onto the partition column. This single site covers
+ # ad-hoc filters, dashboard native filters and cross-filters,
+ # because they all arrive as entries in `filter`. `eq` rather
+ # than the raw `val`: it is the value the real predicate uses.
+ # `TEMPORAL_RANGE` is collected in its own branch below, where
+ # the range has been resolved into a pair of bounds.
+ #
+ # A grain makes the real predicate compare the *truncated*
+ # column, so the raw value no longer describes the rows it
+ # matches: drill-to-detail sends `==` on a bucket start, which
+ # every row in the bucket satisfies once truncated. Mirroring
it
+ # raw would keep only the bucket's first instant.
+ #
+ # Grained ranges do mirror, by widening (see the TEMPORAL_RANGE
+ # branch below). The same trick is deferred rather than unsafe
+ # here: `==` on a bucket would have to become a two-sided
range,
+ # which needs a monotonic transform -- `EQUALS` mirrors without
+ # one today -- and a parse back into a datetime; and a grained
+ # `IN` would need a union of buckets, which this sink cannot
+ # express because its entries are AND-ed.
+ if (
+ col_obj is not None
+ and op != utils.FilterOperator.TEMPORAL_RANGE
+ and not filter_grain
+ ):
+ # Window-independent, so both sinks get it. The subquery
+ # filters a different time window but the same everything
+ # else, and this filter is in `where_clause_and` for both.
+ for sink in (partition_mirror, partition_mirror_inner):
+ self._collect_partition_mirror_filter(
Review Comment:
Fixed. `_in_column_number_type` probes at the value the column would hold
rather than the value as submitted, so the mirror is taken at the same
comparator the original predicate normalizes to.
##########
superset/models/helpers.py:
##########
@@ -5041,6 +5688,26 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
)
if _main_dttm_filter is not None:
time_filters.append(_main_dttm_filter)
+ # This block filters `main_dttm_col`, a *different* column
+ # from `dttm_col`. Checking only `dttm_col` below would
miss
+ # it when the mapping tracks the main datetime column.
+ self._collect_partition_mirror_range(
+ partition_mapping,
+ partition_mirror,
+ self.main_dttm_col,
+ from_dttm,
+ to_dttm,
+ )
+ if not db_engine_spec.time_groupby_inline:
+ # Only where the subquery actually gets a time filter
+ # -- see the longer note on the `dttm_col` mirror
below.
+ self._collect_partition_mirror_range(
+ partition_mapping,
+ partition_mirror_inner,
Review Comment:
Fixed. The `main_dttm_col` mirror is collected for the outer query only,
with no inner counterpart — the subquery's time predicate is built on
`dttm_col`, which that branch has already established is a different column, so
there is no inner time predicate for a mirror to stand in for.
##########
superset/models/helpers.py:
##########
@@ -5317,6 +5717,74 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
db_extra=self.db_extra,
)
+ # Hoisted above the mirror collection below, which has to
+ # record the values the real predicate *binds*. SQLAlchemy
+ # infers an `IN` bind's type from the first element, so a mixed
+ # int/float list has to be widened before binding or every
+ # later float is truncated (see #33206). The `IN` branch did
+ # this itself, 100-odd lines down and by rebinding `eq` -- so
+ # the mirror had already copied the unwidened values and probed
+ # the transform at a number the query never compares.
+ #
+ # Scoped to the operators whose branch did it, so that moving
+ # the call changes when it runs and not what it applies to:
+ # `CONTAINS_ANY`/`CONTAINS_ALL` are list targets too and were
+ # never widened.
+ if (
+ target_generic_type == utils.GenericDataType.NUMERIC
+ and op in {utils.FilterOperator.IN,
utils.FilterOperator.NOT_IN}
+ and isinstance(eq, (list, tuple))
+ ):
+ eq = normalize_mixed_numbers(eq)
+
+ # Mirror onto the partition column. This single site covers
+ # ad-hoc filters, dashboard native filters and cross-filters,
+ # because they all arrive as entries in `filter`. `eq` rather
+ # than the raw `val`: it is the value the real predicate binds,
+ # which is only true because the numeric widening above now
+ # runs before this rather than inside the `IN` branch below.
+ # `TEMPORAL_RANGE` is collected in its own branch below, where
+ # the range has been resolved into a pair of bounds.
+ #
+ # A grain makes the real predicate compare the *truncated*
+ # column, so the raw value no longer describes the rows it
+ # matches: drill-to-detail sends `==` on a bucket start, which
+ # every row in the bucket satisfies once truncated. Mirroring
it
+ # raw would keep only the bucket's first instant.
+ #
+ # Grained ranges do mirror, by widening (see the TEMPORAL_RANGE
+ # branch below). The same trick is deferred rather than unsafe
+ # here: `==` on a bucket would have to become a two-sided
range,
+ # which needs a monotonic transform -- `EQUALS` mirrors without
+ # one today -- and a parse back into a datetime; and a grained
+ # `IN` would need a union of buckets, which this sink cannot
+ # express because its entries are AND-ed.
+ if (
+ col_obj is not None
+ and op != utils.FilterOperator.TEMPORAL_RANGE
+ and not filter_grain
+ ):
+ mirror_value: Any = eq
Review Comment:
Fixed. `_as_probe_input` runs first, so engine SQL from
`filter_values_handler` arrives as a `RawProbeValue` that `build_probe_sql`
substitutes rather than binds — the same wrapping preview applies.
##########
superset/models/helpers.py:
##########
@@ -5602,6 +5831,11 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
# col_obj is None and sqla_col is None - column not found!
# Silently skip - this can happen for removed columns or
invalid filters
pass
+ if partition_mapping is not None:
+ where_clause_and += self._build_partition_mirror_predicates(
Review Comment:
Fixed. Each window builds its own bounds: the inner sink is collected with
`inner_from_dttm or from_dttm` and `inner_to_dttm or to_dttm`, so a
relative-comparison subquery no longer ranks one window's rows against another
window's partition keys.
--
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]
