aminghadersohi commented on code in PR #45571:
URL: https://github.com/apache/superset/pull/45571#discussion_r4346121753
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/EchartsSunburst.tsx:
##########
@@ -120,26 +123,53 @@ export default function EchartsSunburst(props:
SunburstTransformedProps) {
const drillToDetailFilters: BinaryQueryObjectFilterClause[] = [];
const drillByFilters: BinaryQueryObjectFilterClause[] = [];
if (columns?.length) {
- treePath.forEach((path, i) =>
- drillToDetailFilters.push({
- col: columns[i],
- op: '==',
- val: records[i],
- formattedVal: path,
- }),
- );
- const val = treePath[treePath.length - 1];
- drillByFilters.push({
- col: columns[treePath.length - 1],
- op: '==',
- val,
- formattedVal: formatSeriesName(val, {
- timeFormatter: getTimeFormatter(formData.dateFormat),
- numberFormatter: getNumberFormatter(formData.numberFormat),
- coltype:
- coltypeMapping?.[getColumnLabel(columns[treePath.length - 1])],
- }),
+ const timeGrain = (formData.extraFormData?.time_grain_sqla ??
+ formData.timeGrainSqla) as TimeGranularity | undefined;
+ treePath.forEach((path, i) => {
+ // Range filters apply the dataset timezone/offset to both bucket
+ // bounds. Equality on a truncated column bypasses that handling.
+ const temporalFilter =
+ timeGrain &&
+ isGrainedLevel(
+ columns[i],
+ formData.temporalColumnsLookup,
+ formData.granularitySqla,
+ )
+ ? getTemporalXAxisDrillByFilter(
+ columns[i],
+ records[i],
+ timeGrain,
+ path,
+ )
+ : undefined;
+ drillToDetailFilters.push(
+ temporalFilter ?? {
+ col: columns[i],
+ op: '==',
+ val: records[i],
+ formattedVal: path,
+ },
+ );
+ if (temporalFilter && i === treePath.length - 1) {
+ drillByFilters.push(temporalFilter);
+ }
Review Comment:
The behavior described is accurate, but this PR does not introduce it. Drill
by on Sunburst has only ever filtered the clicked level, for every ancestor
type: on master before this PR (EchartsSunburst.tsx:131-142 at f9a0d8af49) the
drill-by filter list is only `columns[treePath.length - 1] == value`, so a
categorical child under a categorical ancestor also drops that ancestor. This
PR keeps that contract and only changes the clicked level's own filter when
that level is a grained temporal column (EchartsSunburst.tsx:153-172 at
9d8c2acb81, pinned by `drill-by on a grained temporal segment uses its raw
bucket instead of its label` and `categorical drill-by and root detail filters
retain their behavior` in test/Sunburst/EchartsSunburst.test.tsx).
https://github.com/apache/superset/pull/45598 keeps the same last-level-only
rule.
Carrying temporal ancestors alone would make Drill by keep a date bucket
while dropping a region above it, which is a separate change to Drill by
semantics for all hierarchy levels.
Remaining: Drill by from a child segment does not carry any ancestor filters
(temporal or categorical). Not changed in this PR; I am leaving this thread
open for the maintainers to decide whether Drill by should carry ancestor
filters.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Sunburst/buildQuery.ts:
##########
@@ -16,14 +16,83 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { buildQueryContext, QueryFormData } from '@superset-ui/core';
+import {
+ AdhocColumn,
+ buildQueryContext,
+ ensureIsArray,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+} from '@superset-ui/core';
+
+// A hierarchy level takes the time grain when it is a physical column that the
+// temporal lookup (or the legacy ``granularity_sqla``) marks temporal.
+export function isGrainedLevel(
+ column: QueryFormColumn,
+ temporalColumnsLookup: Record<string, boolean> | undefined,
+ granularitySqla: string | undefined,
+): boolean {
+ return (
+ isPhysicalColumn(column) &&
+ (!!temporalColumnsLookup?.[column] || granularitySqla === column)
+ );
+}
+
+// Wrap each temporal hierarchy level in the BASE_AXIS column reference that
+// carries the time grain, the shape ``normalizeTimeColumn`` uses for a
physical
+// x-axis. Non-temporal levels are left untouched.
+function applyTimeGrain(
+ columns: QueryFormColumn[],
+ formData: QueryFormData,
+ timeGrain: string | undefined,
+): { columns: QueryFormColumn[]; grained: boolean } {
+ let grained = false;
+ const mapped = columns.map(column => {
+ if (
+ timeGrain &&
+ isPhysicalColumn(column) &&
+ isGrainedLevel(
+ column,
+ formData.temporal_columns_lookup,
+ formData.granularity_sqla,
+ )
+ ) {
+ grained = true;
+ return {
+ timeGrain,
+ columnType: 'BASE_AXIS',
+ sqlExpression: column,
+ label: column,
+ expressionType: 'SQL',
+ isColumnReference: true,
+ } as AdhocColumn;
Review Comment:
Confirmed at 9d8c2acb81: `getCrossFilterDataMask` emits `{col:
columns[treePath.length - 1], op: '==', val: <formatted segment name>}`
(EchartsSunburst.tsx:78-96), so a grained temporal segment cross-filters by its
label. This PR does not touch the cross-filter path; its scope is Explore
keeping the saved grain plus the drill ranges.
The cross-filter fix is in https://github.com/apache/superset/pull/45598:
the click path there builds the filter from the raw record with
`getSunburstFilter`, which returns a `TEMPORAL_RANGE` over the bucket for a
grained temporal level and also sets `time_range` on `extraFormData`. Resolving
here as covered by that PR.
--
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]
