ariel-sofi commented on code in PR #42477: URL: https://github.com/apache/superset/pull/42477#discussion_r3657249588
########## superset-frontend/plugins/plugin-chart-echarts/test/Sankey/buildQuery.test.ts: ########## @@ -0,0 +1,79 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import buildQuery from '../../src/Sankey/buildQuery'; +import { SankeyFormData } from '../../src/Sankey/types'; + +const baseFormData: SankeyFormData = { + colorScheme: 'supersetColors', + datasource: '1__table', + metric: 'count', + source: 'source_col', + target: 'target_col', + viz_type: 'sankey_v2', +}; + +test('two-column form data builds a single source/target groupby', () => { + const [query] = buildQuery(baseFormData).queries; + expect(query.groupby).toEqual(['source_col', 'target_col']); +}); + +test('intermediate levels are included in order between source and target', () => { + const [query] = buildQuery({ + ...baseFormData, + intermediate_levels: ['level_1', 'level_2'], + }).queries; + expect(query.groupby).toEqual([ + 'source_col', + 'level_1', + 'level_2', + 'target_col', + ]); Review Comment: Not reproducible — `src/Sankey/buildQuery.ts` **is** in this PR (19 lines changed in 0c49f68), and it builds `groupby` from all three fields: ```ts const groupby = [source, ...ensureIsArray(intermediate_levels), target]; ``` Both tests pass against it: ``` PASS plugins/plugin-chart-echarts/test/Sankey/buildQuery.test.ts Tests: 17 passed, 17 total ``` Looks like the diff you were given contained the test files without their production counterparts. ########## superset-frontend/plugins/plugin-chart-echarts/test/Sankey/transformProps.test.ts: ########## @@ -0,0 +1,176 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { ChartProps, DataRecord } from '@superset-ui/core'; +import { supersetTheme } from '@apache-superset/core/theme'; +import type { SankeySeriesOption } from 'echarts/charts'; +import transformProps from '../../src/Sankey/transformProps'; +import { SankeyChartProps } from '../../src/Sankey/types'; + +type SankeyNode = { name: string; depth?: number }; +type SankeyLink = { source: string; target: string; value: number }; + +const getSeries = (props: ChartProps): SankeySeriesOption => { + const { echartOptions } = transformProps(props as SankeyChartProps); + return (echartOptions as { series: SankeySeriesOption }).series; +}; + +const getNodes = (props: ChartProps) => getSeries(props).data as SankeyNode[]; + +const getLinks = (props: ChartProps) => getSeries(props).links as SankeyLink[]; + +const makeProps = ( + formDataOverrides: Record<string, unknown>, + data: DataRecord[], +) => + new ChartProps({ + formData: { + colorScheme: 'supersetColors', + datasource: '1__table', + metric: 'count', + source: 'source_col', + target: 'target_col', + vizType: 'sankey_v2', + ...formDataOverrides, + }, + width: 800, + height: 600, + queriesData: [{ data }], + theme: supersetTheme, + }); + +test('two-column mode emits raw pairwise links and node names', () => { + const props = makeProps({}, [ + { source_col: 'a', target_col: 'b', count: 10 }, + { source_col: 'b', target_col: 'c', count: 5 }, + ]); + expect(getLinks(props)).toEqual([ + { source: 'a', target: 'b', value: 10 }, + { source: 'b', target: 'c', value: 5 }, + ]); + const nodes = getNodes(props); + expect(nodes.map(node => node.name).sort()).toEqual(['a', 'b', 'c']); + // no level prefixes and no depth pinning: cross-row chaining (a→b→c) + // must keep working for edge-list datasets + nodes.forEach(node => expect(node.depth).toBeUndefined()); +}); + +test('three-column mode chains adjacent pairs with level-prefixed names', () => { + const props = makeProps({ intermediateLevels: ['mid_col'] }, [ + { source_col: 'a', mid_col: 'm', target_col: 'z', count: 10 }, + ]); + expect(getLinks(props)).toEqual([ + { source: '0\0a', target: '1\0m', value: 10 }, + { source: '1\0m', target: '2\0z', value: 10 }, + ]); Review Comment: Not reproducible — `src/Sankey/transformProps.ts` **is** in this PR (145 lines changed in 0c49f68). It emits adjacent-pair links, level-prefixed node names and `depth`, which is exactly the node-merge/DAG-cycle problem you describe: ```ts const isMultiLevel = levelColumns.length > 2; const makeNodeName = (levelIndex, value) => isMultiLevel ? `${levelIndex}${LEVEL_DELIMITER}${display}` : display; ``` The transform tests pass against it: ``` PASS plugins/plugin-chart-echarts/test/Sankey/transformProps.test.ts Tests: 17 passed, 17 total ``` Same root cause as the sibling comment on `buildQuery.test.ts`: the diff under review appears to have included the test files without their production counterparts. -- 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]
