You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/08/23 14:25:43 UTC
[GitHub] [superset] zhaoyongjie opened a new pull request, #21163: feat: time grain into column
zhaoyongjie opened a new pull request, #21163:
URL: https://github.com/apache/superset/pull/21163
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR intends to set `time grain` from the `column config`, and this one focus on the backend implementation. I will open separated PR to implement the frontend.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
Pure backend, so no screenshot.
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
passed all CI
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r961412641
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/sections.tsx:
##########
@@ -38,6 +38,19 @@ export const legacyTimeseriesTime: ControlPanelSectionConfig = {
],
};
+export const echartsTimeseriesTime: ControlPanelSectionConfig = {
Review Comment:
Would it make to call this `genericTime` or `genericTimeSection`? Just to decouple it more from the ECharts plugin.
##########
superset/utils/core.py:
##########
@@ -1263,6 +1263,17 @@ def is_adhoc_column(column: Column) -> TypeGuard[AdhocColumn]:
return isinstance(column, dict)
+def get_axis_column(columns: Optional[List[Column]]) -> Optional[AdhocColumn]:
Review Comment:
Should probably be renamed if we go with `BASE_AXIS`
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
Review Comment:
This can be very subjective and different for different people, but in general I find narrower and longer code snippets more readable than wider and shorter. In this case I find @michael-s-molina 's destructuring approach more readable. Having said that I'm ok with both approaches, as long as the function/file follows a similar pattern throughout.
##########
superset/connectors/sqla/utils.py:
##########
@@ -137,7 +139,7 @@ def get_virtual_table_metadata(dataset: "SqlaTable") -> List[ResultSetColumnType
try:
with closing(engine.raw_connection()) as conn:
cursor = conn.cursor()
- query = dataset.database.apply_limit_to_sql(statements[0])
+ query = dataset.database.apply_limit_to_sql(statements[0], limit=1)
Review Comment:
Nice improvement π
##########
superset-frontend/packages/superset-ui-core/src/query/types/Column.ts:
##########
@@ -27,6 +27,8 @@ export interface AdhocColumn {
optionName?: string;
sqlExpression: string;
expressionType: 'SQL';
+ columnType?: 'AXIS' | 'SERIES';
Review Comment:
To be more explicit, I think `BASE_AXIS` could be more clear, as `AXIS` could refer to the either axis (it could be argued that `SERIES` is on the `VALUE_AXIS` or similar).
```suggestion
columnType?: 'BASE_AXIS' | 'SERIES';
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r1017498065
##########
UPDATING.md:
##########
@@ -27,6 +27,8 @@ assists people when migrating to a new version.
- [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
- [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return `None`.
- [21002](https://github.com/apache/superset/pull/21002): Support Python 3.10 and bump pandas 1.4 and pyarrow 6.
+- [21163](https://github.com/apache/superset/pull/21163): When `GENERIC_CHART_AXES` feature flags set to `True`, the Time Grain control will move below the X-Axis control.
Review Comment:
@john-bodley update the document at [PR](https://github.com/apache/superset/pull/22074)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233851049
/testenv up GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true DASHBOARD_CROSS_FILTERS=true
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] michael-s-molina commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
michael-s-molina commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r960897709
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -122,4 +123,50 @@ describe('buildQueryContext', () => {
},
]);
});
+ it('should call normalizeTimeColumn if enable GENERIC_CHART_AXES', () => {
Review Comment:
```suggestion
it('should call normalizeTimeColumn if GENERIC_CHART_AXES is enabled', () => {
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
+ col =>
+ (isPhysicalColumn(col) &&
+ isPhysicalColumn(formData.x_axis) &&
+ col === formData.x_axis) ||
+ (isAdhocColumn(col) &&
+ isAdhocColumn(formData.x_axis) &&
+ col.sqlExpression === formData.x_axis.sqlExpression),
+ );
+ if (
+ axisIdx !== undefined &&
+ axisIdx > -1 &&
+ formData.x_axis &&
+ Array.isArray(queryObject.columns)
+ ) {
+ if (isAdhocColumn(queryObject.columns[axisIdx])) {
+ mutatedColumns[axisIdx] = {
+ timeGrain: queryObject?.extras?.time_grain_sqla,
Review Comment:
```suggestion
timeGrain: extras?.time_grain_sqla,
```
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -17,6 +17,7 @@
* under the License.
*/
import { buildQueryContext } from '@superset-ui/core';
+import * as queryMoudle from '../../src/query/normalizeTimeColumn';
Review Comment:
```suggestion
import * as queryModule from '../../src/query/normalizeTimeColumn';
```
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -122,4 +123,50 @@ describe('buildQueryContext', () => {
},
]);
});
+ it('should call normalizeTimeColumn if enable GENERIC_CHART_AXES', () => {
+ // @ts-ignore
+ const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: true,
+ },
+ }));
+ const spyNormalizeTimeColumn = jest.spyOn(
+ queryMoudle,
+ 'normalizeTimeColumn',
+ );
+
+ buildQueryContext(
+ {
+ datasource: '5__table',
+ viz_type: 'table',
+ },
+ () => [{}],
+ );
+ expect(spyNormalizeTimeColumn).toBeCalled();
+ spy.mockRestore();
+ spyNormalizeTimeColumn.mockRestore();
+ });
+ it("shouldn't call normalizeTimeColumn if disable GENERIC_CHART_AXES", () => {
Review Comment:
```suggestion
it("shouldn't call normalizeTimeColumn if GENERIC_CHART_AXES is disabled", () => {
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
Review Comment:
```suggestion
if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) || !formData.x_axis) {
return queryObject;
}
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
+ col =>
+ (isPhysicalColumn(col) &&
+ isPhysicalColumn(formData.x_axis) &&
+ col === formData.x_axis) ||
+ (isAdhocColumn(col) &&
+ isAdhocColumn(formData.x_axis) &&
+ col.sqlExpression === formData.x_axis.sqlExpression),
+ );
+ if (
+ axisIdx !== undefined &&
+ axisIdx > -1 &&
+ formData.x_axis &&
+ Array.isArray(queryObject.columns)
+ ) {
+ if (isAdhocColumn(queryObject.columns[axisIdx])) {
Review Comment:
```suggestion
if (isAdhocColumn(columns[axisIdx])) {
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx:
##########
@@ -323,6 +325,21 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = {
mapStateToProps: ({ datasource }) => ({
choices: (datasource as Dataset)?.time_grain_sqla || null,
}),
+ visibility: ({ controls }) => {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return true;
+ }
+
+ const xAxisValue = controls?.x_axis?.value;
+ if (xAxisValue === undefined || isAdhocColumn(xAxisValue)) {
+ return true;
+ }
+ if (isPhysicalColumn(xAxisValue)) {
+ const options = controls?.x_axis?.options ?? {};
+ return options[xAxisValue]?.is_dttm;
Review Comment:
```suggestion
return xAxis?.options?.[xAxisValue]?.is_dttm ?? false;
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
Review Comment:
```suggestion
const { columns, extras } = queryObject;
const mutatedColumns: QueryFormColumn[] = [...(columns || [])];
const axisIdx = columns?.findIndex(
```
##########
superset/utils/core.py:
##########
@@ -1263,6 +1263,19 @@ def is_adhoc_column(column: Column) -> TypeGuard[AdhocColumn]:
return isinstance(column, dict)
+def get_axis_column(columns: Optional[List[Column]]) -> Optional[AdhocColumn]:
+ if columns is None:
+ return None
+ axis_cols = [
+ col
+ for col in columns
+ if is_adhoc_column(col)
+ and col.get("columnType")
+ and col.get("columnType") == "AXIS"
Review Comment:
```suggestion
and col.get("columnType") == "AXIS"
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
+ col =>
+ (isPhysicalColumn(col) &&
+ isPhysicalColumn(formData.x_axis) &&
+ col === formData.x_axis) ||
+ (isAdhocColumn(col) &&
+ isAdhocColumn(formData.x_axis) &&
+ col.sqlExpression === formData.x_axis.sqlExpression),
+ );
+ if (
+ axisIdx !== undefined &&
+ axisIdx > -1 &&
+ formData.x_axis &&
+ Array.isArray(queryObject.columns)
+ ) {
+ if (isAdhocColumn(queryObject.columns[axisIdx])) {
+ mutatedColumns[axisIdx] = {
+ timeGrain: queryObject?.extras?.time_grain_sqla,
+ columnType: 'AXIS',
+ ...(queryObject.columns[axisIdx] as AdhocColumn),
+ };
+ } else {
+ mutatedColumns[axisIdx] = {
+ timeGrain: queryObject?.extras?.time_grain_sqla,
Review Comment:
```suggestion
timeGrain: extras?.time_grain_sqla,
```
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
+ col =>
+ (isPhysicalColumn(col) &&
+ isPhysicalColumn(formData.x_axis) &&
+ col === formData.x_axis) ||
+ (isAdhocColumn(col) &&
+ isAdhocColumn(formData.x_axis) &&
+ col.sqlExpression === formData.x_axis.sqlExpression),
+ );
+ if (
+ axisIdx !== undefined &&
+ axisIdx > -1 &&
+ formData.x_axis &&
+ Array.isArray(queryObject.columns)
Review Comment:
```suggestion
Array.isArray(columns)
```
##########
superset-frontend/packages/superset-ui-core/test/query/normalizeTimeColumn.test.ts:
##########
@@ -0,0 +1,247 @@
+/**
+ * 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 {
+ normalizeTimeColumn,
+ QueryObject,
+ SqlaFormData,
+} from '@superset-ui/core';
+
+describe('disabled GENERIC_CHART_AXES', () => {
+ let windowSpy: any;
+
+ beforeAll(() => {
+ // @ts-ignore
+ windowSpy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: false,
+ },
+ }));
+ });
+
+ afterAll(() => {
+ windowSpy.mockRestore();
+ });
+
+ it('should return original QueryObject if disabled GENERIC_CHART_AXES', () => {
+ const formData: SqlaFormData = {
+ datasource: '5__table',
+ viz_type: 'table',
+ granularity: 'time_column',
+ time_grain_sqla: 'P1Y',
+ time_range: '1 year ago : 2013',
+ columns: ['col1'],
+ metrics: ['count(*)'],
+ x_axis: 'time_column',
+ };
+ const query: QueryObject = {
+ datasource: '5__table',
+ viz_type: 'table',
+ granularity: 'time_column',
+ extras: {
+ time_grain_sqla: 'P1Y',
+ },
+ time_range: '1 year ago : 2013',
+ orderby: [['count(*)', true]],
+ columns: ['col1'],
+ metrics: ['count(*)'],
+ is_timeseries: true,
+ };
+ expect(normalizeTimeColumn(formData, query)).toEqual(query);
+ });
+});
+
+describe('enabled GENERIC_CHART_AXES', () => {
+ let windowSpy: any;
+
+ beforeAll(() => {
+ // @ts-ignore
+ windowSpy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: true,
+ },
+ }));
+ });
+
+ afterAll(() => {
+ windowSpy.mockRestore();
+ });
+
+ it('should return original QueryObject if x_axis is empty', () => {
+ const formData: SqlaFormData = {
+ datasource: '5__table',
+ viz_type: 'table',
+ granularity: 'time_column',
+ time_grain_sqla: 'P1Y',
+ time_range: '1 year ago : 2013',
+ columns: ['col1'],
+ metrics: ['count(*)'],
+ };
+ const query: QueryObject = {
+ datasource: '5__table',
+ viz_type: 'table',
+ granularity: 'time_column',
+ extras: {
+ time_grain_sqla: 'P1Y',
+ },
+ time_range: '1 year ago : 2013',
+ orderby: [['count(*)', true]],
+ columns: ['col1'],
+ metrics: ['count(*)'],
+ is_timeseries: true,
+ };
+ expect(normalizeTimeColumn(formData, query)).toEqual(query);
+ });
+
+ it('should support x-axis and granularity to use different column', () => {
Review Comment:
```suggestion
it('should support different columns for x-axis and granularity', () => {
```
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -122,4 +123,50 @@ describe('buildQueryContext', () => {
},
]);
});
+ it('should call normalizeTimeColumn if enable GENERIC_CHART_AXES', () => {
+ // @ts-ignore
+ const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: true,
+ },
+ }));
+ const spyNormalizeTimeColumn = jest.spyOn(
+ queryMoudle,
Review Comment:
```suggestion
queryModule,
```
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx:
##########
@@ -323,6 +325,21 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = {
mapStateToProps: ({ datasource }) => ({
choices: (datasource as Dataset)?.time_grain_sqla || null,
}),
+ visibility: ({ controls }) => {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return true;
+ }
+
+ const xAxisValue = controls?.x_axis?.value;
Review Comment:
```suggestion
const xAxis = controls?.x_axis;
const xAxisValue = xAxis?.value;
```
##########
tests/integration_tests/query_context_tests.py:
##########
@@ -728,3 +730,183 @@ def test_get_label_map(app_context, virtual_dataset_comma_in_column_value):
"count, col2, row2": ["count", "col2, row2"],
"count, col2, row3": ["count", "col2, row3"],
}
+
+
+def test_time_column_with_time_grain(app_context, physical_dataset):
Review Comment:
Nice set of tests!
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
+ col =>
+ (isPhysicalColumn(col) &&
+ isPhysicalColumn(formData.x_axis) &&
+ col === formData.x_axis) ||
+ (isAdhocColumn(col) &&
+ isAdhocColumn(formData.x_axis) &&
+ col.sqlExpression === formData.x_axis.sqlExpression),
+ );
+ if (
+ axisIdx !== undefined &&
+ axisIdx > -1 &&
+ formData.x_axis &&
+ Array.isArray(queryObject.columns)
+ ) {
+ if (isAdhocColumn(queryObject.columns[axisIdx])) {
+ mutatedColumns[axisIdx] = {
+ timeGrain: queryObject?.extras?.time_grain_sqla,
+ columnType: 'AXIS',
+ ...(queryObject.columns[axisIdx] as AdhocColumn),
Review Comment:
```suggestion
...(columns[axisIdx] as AdhocColumn),
```
##########
superset-frontend/packages/superset-ui-core/test/query/buildQueryContext.test.ts:
##########
@@ -122,4 +123,50 @@ describe('buildQueryContext', () => {
},
]);
});
+ it('should call normalizeTimeColumn if enable GENERIC_CHART_AXES', () => {
+ // @ts-ignore
+ const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: true,
+ },
+ }));
+ const spyNormalizeTimeColumn = jest.spyOn(
+ queryMoudle,
+ 'normalizeTimeColumn',
+ );
+
+ buildQueryContext(
+ {
+ datasource: '5__table',
+ viz_type: 'table',
+ },
+ () => [{}],
+ );
+ expect(spyNormalizeTimeColumn).toBeCalled();
+ spy.mockRestore();
+ spyNormalizeTimeColumn.mockRestore();
+ });
+ it("shouldn't call normalizeTimeColumn if disable GENERIC_CHART_AXES", () => {
+ // @ts-ignore
+ const spy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: false,
+ },
+ }));
+ const spyNormalizeTimeColumn = jest.spyOn(
+ queryMoudle,
Review Comment:
```suggestion
queryModule,
```
##########
superset-frontend/packages/superset-ui-core/test/query/normalizeTimeColumn.test.ts:
##########
@@ -0,0 +1,247 @@
+/**
+ * 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 {
+ normalizeTimeColumn,
+ QueryObject,
+ SqlaFormData,
+} from '@superset-ui/core';
+
+describe('disabled GENERIC_CHART_AXES', () => {
+ let windowSpy: any;
+
+ beforeAll(() => {
+ // @ts-ignore
+ windowSpy = jest.spyOn(window, 'window', 'get').mockImplementation(() => ({
+ featureFlags: {
+ GENERIC_CHART_AXES: false,
+ },
+ }));
+ });
+
+ afterAll(() => {
+ windowSpy.mockRestore();
+ });
+
+ it('should return original QueryObject if disabled GENERIC_CHART_AXES', () => {
Review Comment:
```suggestion
it('should return original QueryObject if GENERIC_CHART_AXES is disabled', () => {
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233883161
@zhaoyongjie Ephemeral environment spinning up at http://34.217.174.178:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] john-bodley commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
john-bodley commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r1015963697
##########
UPDATING.md:
##########
@@ -27,6 +27,8 @@ assists people when migrating to a new version.
- [20606](https://github.com/apache/superset/pull/20606): When user clicks on chart title or "Edit chart" button in Dashboard page, Explore opens in the same tab. Clicking while holding cmd/ctrl opens Explore in a new tab. To bring back the old behaviour (always opening Explore in a new tab), flip feature flag `DASHBOARD_EDIT_CHART_IN_NEW_TAB` to `True`.
- [20799](https://github.com/apache/superset/pull/20799): Presto and Trino engine will now display tracking URL for running queries in SQL Lab. If for some reason you don't want to show the tracking URL (for example, when your data warehouse hasn't enable access for to Presto or Trino UI), update `TRACKING_URL_TRANSFORMER` in `config.py` to return `None`.
- [21002](https://github.com/apache/superset/pull/21002): Support Python 3.10 and bump pandas 1.4 and pyarrow 6.
+- [21163](https://github.com/apache/superset/pull/21163): When `GENERIC_CHART_AXES` feature flags set to `True`, the Time Grain control will move below the X-Axis control.
Review Comment:
@zhaoyongjie would you mind expanding on this bullet point? From reading through the PR description et al. it seems that the biggest change is that the time grain is now decoupled from the time filter column. As it currently reads now "... will move below the X-Axis control." it sounds like it's purely a cosmetic change as opposed to a scoping change.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r961267261
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
Review Comment:
IMO, avoiding redundant local variables is for readability, in this case, readability refers to the intermediate variables that do not bring benefits but add cognitive load -- I have to remember that `columns` and `extra` is from `queryObject` other than otherwhere.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233861020
@zhaoyongjie Ephemeral environment spinning up at http://34.210.85.10:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1235415212
@zhaoyongjie Ephemeral environment spinning up at http://35.91.72.50:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] jinghua-qa commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
jinghua-qa commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1239068110
Tested in ephemeral env, and the new feature LGTM.
However i have found 2 issues that also in master branch, i will file tickets to follow up.
1, default datetime is not autoselected as the x-axis when multiple column set is temporal
https://user-images.githubusercontent.com/81597121/188827803-5e994ce4-0413-4e74-a22c-d9221934c378.mov
2, when db=sqlite, and use custom sql in x-axis, time grain did not apply
https://user-images.githubusercontent.com/81597121/188827265-26e35d57-dd74-4006-881b-3a151457cbdf.mov
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #21163: feat: time grain into column
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1224174938
# [Codecov](https://codecov.io/gh/apache/superset/pull/21163?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#21163](https://codecov.io/gh/apache/superset/pull/21163?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (b1bf7d6) into [master](https://codecov.io/gh/apache/superset/commit/51e567ffef684b5e3fb9e5bdfaccd9ad2777f4c8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (51e567f) will **decrease** coverage by `11.43%`.
> The diff coverage is `75.00%`.
```diff
@@ Coverage Diff @@
## master #21163 +/- ##
===========================================
- Coverage 66.33% 54.90% -11.44%
===========================================
Files 1779 1779
Lines 67835 67839 +4
Branches 7237 7237
===========================================
- Hits 44998 37246 -7752
- Misses 20985 28741 +7756
Partials 1852 1852
```
| Flag | Coverage Ξ | |
|---|---|---|
| hive | `53.18% <75.00%> (+<0.01%)` | :arrow_up: |
| mysql | `?` | |
| postgres | `?` | |
| presto | `53.08% <75.00%> (+<0.01%)` | :arrow_up: |
| python | `57.83% <75.00%> (-23.69%)` | :arrow_down: |
| sqlite | `?` | |
| unit | `50.74% <50.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/21163?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [superset/connectors/sqla/models.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvY29ubmVjdG9ycy9zcWxhL21vZGVscy5weQ==) | `77.78% <50.00%> (-12.54%)` | :arrow_down: |
| [superset/superset\_typing.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvc3VwZXJzZXRfdHlwaW5nLnB5) | `98.18% <100.00%> (+0.06%)` | :arrow_up: |
| [superset/utils/dashboard\_import\_export.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvdXRpbHMvZGFzaGJvYXJkX2ltcG9ydF9leHBvcnQucHk=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset/key\_value/commands/update.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL3VwZGF0ZS5weQ==) | `0.00% <0.00%> (-88.89%)` | :arrow_down: |
| [superset/key\_value/commands/delete.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZS5weQ==) | `0.00% <0.00%> (-85.30%)` | :arrow_down: |
| [superset/key\_value/commands/delete\_expired.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQva2V5X3ZhbHVlL2NvbW1hbmRzL2RlbGV0ZV9leHBpcmVkLnB5) | `0.00% <0.00%> (-80.77%)` | :arrow_down: |
| [superset/dashboards/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGFzaGJvYXJkcy9jb21tYW5kcy9pbXBvcnRlcnMvdjAucHk=) | `15.62% <0.00%> (-76.25%)` | :arrow_down: |
| [superset/datasets/commands/update.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvdXBkYXRlLnB5) | `25.00% <0.00%> (-69.05%)` | :arrow_down: |
| [superset/datasets/commands/create.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvY3JlYXRlLnB5) | `29.41% <0.00%> (-68.63%)` | :arrow_down: |
| [superset/datasets/commands/importers/v0.py](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQvZGF0YXNldHMvY29tbWFuZHMvaW1wb3J0ZXJzL3YwLnB5) | `24.03% <0.00%> (-67.45%)` | :arrow_down: |
| ... and [280 more](https://codecov.io/gh/apache/superset/pull/21163/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: Weβre building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r954866723
##########
superset/connectors/sqla/utils.py:
##########
@@ -137,7 +139,7 @@ def get_virtual_table_metadata(dataset: "SqlaTable") -> List[ResultSetColumnType
try:
with closing(engine.raw_connection()) as conn:
cursor = conn.cursor()
- query = dataset.database.apply_limit_to_sql(statements[0])
+ query = dataset.database.apply_limit_to_sql(statements[0], limit=1)
Review Comment:
bycatch: fetch virtual table should limit query.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1239070455
@jinghua-qa Thanks for testing, I'm gonna follow up in a separate 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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on a diff in pull request #21163: feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r957482083
##########
tests/integration_tests/query_context_tests.py:
##########
@@ -728,3 +730,183 @@ def test_get_label_map(app_context, virtual_dataset_comma_in_column_value):
"count, col2, row2": ["count", "col2, row2"],
"count, col2, row3": ["count", "col2, row3"],
}
+
+
+def test_time_column_with_time_grain(app_context, physical_dataset):
+ column_on_axis: AdhocColumn = {
+ "label": "I_AM_A_ORIGINAL_COLUMN",
Review Comment:
nit (don't bother changing this unless you happen to have the file open in your IDE π):
```suggestion
"label": "I_AM_AN_ORIGINAL_COLUMN",
```
##########
superset/superset_typing.py:
##########
@@ -55,6 +55,8 @@ class AdhocColumn(TypedDict, total=False):
hasCustomLabel: Optional[bool]
label: Optional[str]
sqlExpression: Optional[str]
+ is_axis: Optional[bool]
+ time_grain: Optional[str]
Review Comment:
nit: these are snake_case, while the old ones are camelCase. It's a matter of taste, of course, but I kinda prefer using camelCase as these kind of originate from the TS codebase.
##########
tests/integration_tests/query_context_tests.py:
##########
@@ -728,3 +730,183 @@ def test_get_label_map(app_context, virtual_dataset_comma_in_column_value):
"count, col2, row2": ["count", "col2, row2"],
"count, col2, row3": ["count", "col2, row3"],
}
+
+
+def test_time_column_with_time_grain(app_context, physical_dataset):
+ column_on_axis: AdhocColumn = {
+ "label": "I_AM_A_ORIGINAL_COLUMN",
+ "sqlExpression": "col5",
+ "time_grain": "P1Y",
+ }
+ adhoc_column: AdhocColumn = {
+ "label": "I_AM_A_TRUNC_COLUMN",
+ "sqlExpression": "col6",
+ "is_axis": True,
+ "time_grain": "P1Y",
+ }
+ qc = QueryContextFactory().create(
+ datasource={
+ "type": physical_dataset.type,
+ "id": physical_dataset.id,
+ },
+ queries=[
+ {
+ "columns": ["col1", column_on_axis, adhoc_column],
+ "metrics": ["count"],
+ "orderby": [["col1", True]],
+ }
+ ],
+ result_type=ChartDataResultType.FULL,
+ force=True,
+ )
+ query_object = qc.queries[0]
+ df = qc.get_df_payload(query_object)["df"]
+ if query_object.datasource.database.backend == "sqlite":
+ # sqlite returns string as timestamp column
+ assert df["I_AM_A_ORIGINAL_COLUMN"][0] == "2000-01-01 00:00:00"
+ assert df["I_AM_A_ORIGINAL_COLUMN"][1] == "2000-01-02 00:00:00"
+ assert df["I_AM_A_TRUNC_COLUMN"][0] == "2002-01-01 00:00:00"
+ assert df["I_AM_A_TRUNC_COLUMN"][1] == "2002-01-01 00:00:00"
+ else:
+ assert df["I_AM_A_ORIGINAL_COLUMN"][0].strftime("%Y-%m-%d") == "2000-01-01"
+ assert df["I_AM_A_ORIGINAL_COLUMN"][1].strftime("%Y-%m-%d") == "2000-01-02"
+ assert df["I_AM_A_TRUNC_COLUMN"][0].strftime("%Y-%m-%d") == "2002-01-01"
+ assert df["I_AM_A_TRUNC_COLUMN"][1].strftime("%Y-%m-%d") == "2002-01-01"
+
+
+def test_non_time_column_with_time_grain(app_context, physical_dataset):
+ qc = QueryContextFactory().create(
+ datasource={
+ "type": physical_dataset.type,
+ "id": physical_dataset.id,
+ },
+ queries=[
+ {
+ "columns": [
+ "col1",
+ {
+ "label": "COL2 ALIAS",
+ "sqlExpression": "col2",
+ "is_axis": True,
+ "time_grain": "P1Y",
+ },
+ ],
+ "metrics": ["count"],
+ "orderby": [["col1", True]],
+ "row_limit": 1,
+ }
+ ],
+ result_type=ChartDataResultType.FULL,
+ force=True,
+ )
+
+ query_object = qc.queries[0]
+ df = qc.get_df_payload(query_object)["df"]
+ assert df["COL2 ALIAS"][0] == "a"
+
+
+def test_special_chars_in_column_name(app_context, physical_dataset):
+ qc = QueryContextFactory().create(
+ datasource={
+ "type": physical_dataset.type,
+ "id": physical_dataset.id,
+ },
+ queries=[
+ {
+ "columns": [
+ "col1",
+ "time column with spaces",
+ {
+ "label": "I_AM_A_TRUNC_COLUMN",
+ "sqlExpression": "time column with spaces",
+ "is_axis": True,
+ "time_grain": "P1Y",
+ },
+ ],
+ "metrics": ["count"],
+ "orderby": [["col1", True]],
+ "row_limit": 1,
+ }
+ ],
+ result_type=ChartDataResultType.FULL,
+ force=True,
+ )
+
+ query_object = qc.queries[0]
+ df = qc.get_df_payload(query_object)["df"]
+ if query_object.datasource.database.backend == "sqlite":
+ # sqlite returns string as timestamp column
+ assert df["time column with spaces"][0] == "2002-01-03 00:00:00"
+ assert df["I_AM_A_TRUNC_COLUMN"][0] == "2002-01-01 00:00:00"
+ else:
+ assert df["time column with spaces"][0].strftime("%Y-%m-%d") == "2002-01-03"
+ assert df["I_AM_A_TRUNC_COLUMN"][0].strftime("%Y-%m-%d") == "2002-01-01"
+
+
+@only_postgresql
Review Comment:
I love this!
##########
superset/connectors/sqla/utils.py:
##########
@@ -147,6 +149,24 @@ def get_virtual_table_metadata(dataset: "SqlaTable") -> List[ResultSetColumnType
return cols
+def get_columns_description(
+ database: Database,
+ query: str,
+) -> List[ResultSetColumnType]:
+ db_engine_spec = database.db_engine_spec
+ try:
+ with closing(database.get_sqla_engine().raw_connection()) as conn:
+ cursor = conn.cursor()
+ query = database.apply_limit_to_sql(query, limit=1)
+ cursor.execute(query)
+ db_engine_spec.execute(cursor, query)
+ result = db_engine_spec.fetch_data(cursor, limit=1)
+ result_set = SupersetResultSet(result, cursor.description, db_engine_spec)
+ return result_set.columns
+ except Exception as ex:
+ raise SupersetGenericDBErrorException(message=str(ex)) from ex
Review Comment:
I've been thinking about this for quite some time, and one way of doing this would be adding a new endpoint to the dataset API, like `[GET] /api/v1/dataset/{pk}/coltype`, that you could send any arbitrary expression to, and it would return the column metadata for it (the `is_dttm` etc that we now get back in the description). Then we could add a button to the adhoc control that makes it possible to fetch the metadata, and if it's temporal, it would expose the time grain dropdown in the popover.
This would make it possible to have different time grains for different columns, and reuse the same logic here (when we still don't have the frontend designs, we could just call the relevant logic from the backend without an API call).
##########
superset/connectors/sqla/utils.py:
##########
@@ -184,12 +204,12 @@ def validate_adhoc_subquery(
@memoized
def get_dialect_name(drivername: str) -> str:
- return SqlaURL(drivername).get_dialect().name
+ return SqlaURL.create(drivername).get_dialect().name
Review Comment:
nice catch
##########
superset/connectors/sqla/models.py:
##########
@@ -1124,7 +1125,25 @@ def adhoc_column_to_sqla(
schema=self.schema,
template_processor=template_processor,
)
- sqla_column = literal_column(expression)
+ col_in_metadata = self.get_column(expression)
+ if col_in_metadata:
+ sqla_column = col_in_metadata.get_sqla_col()
+ is_dttm = col_in_metadata.is_temporal
+ else:
+ sqla_column = literal_column(expression)
+ # probe adhoc column type
+ tbl, _ = self.get_from_clause(template_processor)
+ qry = sa.select([sqla_column]).limit(1).select_from(tbl)
+ sql = self.database.compile_sqla_query(qry)
+ col_desc = get_columns_description(self.database, sql)
+ is_dttm = col_desc[0]["is_dttm"]
+
+ if col.get("is_axis") and col.get("time_grain") and is_dttm:
+ sqla_column = self.db_engine_spec.get_timestamp_expr(
+ sqla_column,
+ None,
+ col.get("time_grain"),
+ )
Review Comment:
I haven't thought quite this far yet, but is there a reason we want to have `is_axis` here in addition to having `time_grain`? I would assume the frontend simply omits `time_grain` if it doesn't want to apply a time grain. Also, mini nit as we're now on 3.8+..
```suggestion
if col.get("is_axis") and time_grain := col.get("time_grain") and is_dttm:
sqla_column = self.db_engine_spec.get_timestamp_expr(
sqla_column,
None,
time_grain,
)
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1235408876
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233795037
/testenv up GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true DASHBOARD_CROSS_FILTERS=true
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r959647659
##########
superset/superset_typing.py:
##########
@@ -55,6 +55,8 @@ class AdhocColumn(TypedDict, total=False):
hasCustomLabel: Optional[bool]
label: Optional[str]
sqlExpression: Optional[str]
+ is_axis: Optional[bool]
+ time_grain: Optional[str]
Review Comment:
nice catch! I am going to change both to camelCase.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: [WIP] feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r959722892
##########
tests/unit_tests/core_tests.py:
##########
@@ -30,7 +30,6 @@
get_metric_names,
get_time_filter_status,
is_adhoc_metric,
- NO_TIME_RANGE,
Review Comment:
bycatch: unused variable
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: [WIP] feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r959726973
##########
superset/connectors/sqla/utils.py:
##########
@@ -147,6 +149,24 @@ def get_virtual_table_metadata(dataset: "SqlaTable") -> List[ResultSetColumnType
return cols
+def get_columns_description(
+ database: Database,
+ query: str,
+) -> List[ResultSetColumnType]:
+ db_engine_spec = database.db_engine_spec
+ try:
+ with closing(database.get_sqla_engine().raw_connection()) as conn:
+ cursor = conn.cursor()
+ query = database.apply_limit_to_sql(query, limit=1)
+ cursor.execute(query)
+ db_engine_spec.execute(cursor, query)
+ result = db_engine_spec.fetch_data(cursor, limit=1)
+ result_set = SupersetResultSet(result, cursor.description, db_engine_spec)
+ return result_set.columns
+ except Exception as ex:
+ raise SupersetGenericDBErrorException(message=str(ex)) from ex
Review Comment:
nice point, I will add this detector API in the separated PR.
##########
superset/connectors/sqla/models.py:
##########
@@ -1124,7 +1125,25 @@ def adhoc_column_to_sqla(
schema=self.schema,
template_processor=template_processor,
)
- sqla_column = literal_column(expression)
+ col_in_metadata = self.get_column(expression)
+ if col_in_metadata:
+ sqla_column = col_in_metadata.get_sqla_col()
+ is_dttm = col_in_metadata.is_temporal
+ else:
+ sqla_column = literal_column(expression)
+ # probe adhoc column type
+ tbl, _ = self.get_from_clause(template_processor)
+ qry = sa.select([sqla_column]).limit(1).select_from(tbl)
+ sql = self.database.compile_sqla_query(qry)
+ col_desc = get_columns_description(self.database, sql)
+ is_dttm = col_desc[0]["is_dttm"]
+
+ if col.get("is_axis") and col.get("time_grain") and is_dttm:
+ sqla_column = self.db_engine_spec.get_timestamp_expr(
+ sqla_column,
+ None,
+ col.get("time_grain"),
+ )
Review Comment:
The `timeGrian` means, this column can use some DB expression (I actually started with a `config` field), but Superset doesn't support other database function mappings now. So I just use the old naming style - 'timeGrain`.
I have changed `is_axis` to the `columnType`. It is easy to extend the type of column using this approach.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r961525704
##########
superset-frontend/packages/superset-ui-core/src/query/normalizeTimeColumn.ts:
##########
@@ -0,0 +1,85 @@
+/**
+ * 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 omit from 'lodash/omit';
+
+import {
+ AdhocColumn,
+ isAdhocColumn,
+ isPhysicalColumn,
+ QueryFormColumn,
+ QueryFormData,
+ QueryObject,
+} from './types';
+import { FeatureFlag, isFeatureEnabled } from '../utils';
+
+export function normalizeTimeColumn(
+ formData: QueryFormData,
+ queryObject: QueryObject,
+): QueryObject {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return queryObject;
+ }
+ if (isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES) && !formData.x_axis) {
+ return queryObject;
+ }
+
+ const mutatedColumns: QueryFormColumn[] = [...(queryObject.columns || [])];
+ const axisIdx = queryObject.columns?.findIndex(
Review Comment:
Thanks both. I have changed to `const { columns: _columns, extras: _extras } = queryObject;`, It's hard to say which approach is better, but the ease of tracking is always my first concern. (especially, in the conversion function)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1231233770
> Pretty cool stuff on the way here..! π Left a few comments. Also, it's slightly difficult to give a good review in the absence of having seen designs or frontend POC code, so if there's anything like that out there I'd love to see it to get a more complete understanding of how this is meant to work eventually.
@villebro Thanks for the review! I will add a frontend part this week for convenience to review.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1235045648
@michael-s-molina Thanks for the review, I have addressed your comments.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233877933
/testenv up FEATURE_GENERIC_CHART_AXES=true FEATURE_DRILL_TO_DETAIL=true FEATURE_DASHBOARD_CROSS_FILTERS=true
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1233804678
@zhaoyongjie Ephemeral environment spinning up at http://52.36.22.204:8080. Credentials are `admin`/`admin`. Please allow several minutes for bootstrapping and startup.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie merged pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie merged PR #21163:
URL: https://github.com/apache/superset/pull/21163
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #21163:
URL: https://github.com/apache/superset/pull/21163#issuecomment-1239073284
Ephemeral environment shutdown and build artifacts deleted.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: [WIP] feat: move time grain into column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r959736868
##########
superset/connectors/sqla/models.py:
##########
@@ -1124,7 +1125,25 @@ def adhoc_column_to_sqla(
schema=self.schema,
template_processor=template_processor,
)
- sqla_column = literal_column(expression)
+ col_in_metadata = self.get_column(expression)
+ if col_in_metadata:
+ sqla_column = col_in_metadata.get_sqla_col()
+ is_dttm = col_in_metadata.is_temporal
+ else:
+ sqla_column = literal_column(expression)
+ # probe adhoc column type
+ tbl, _ = self.get_from_clause(template_processor)
+ qry = sa.select([sqla_column]).limit(1).select_from(tbl)
+ sql = self.database.compile_sqla_query(qry)
+ col_desc = get_columns_description(self.database, sql)
+ is_dttm = col_desc[0]["is_dttm"]
+
+ if col.get("is_axis") and col.get("time_grain") and is_dttm:
+ sqla_column = self.db_engine_spec.get_timestamp_expr(
+ sqla_column,
+ None,
+ col.get("time_grain"),
+ )
Review Comment:
`:=` is an awesome new operator!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r961259026
##########
superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/index.tsx:
##########
@@ -323,6 +325,21 @@ const time_grain_sqla: SharedControlConfig<'SelectControl'> = {
mapStateToProps: ({ datasource }) => ({
choices: (datasource as Dataset)?.time_grain_sqla || null,
}),
+ visibility: ({ controls }) => {
+ if (!isFeatureEnabled(FeatureFlag.GENERIC_CHART_AXES)) {
+ return true;
+ }
+
+ const xAxisValue = controls?.x_axis?.value;
Review Comment:
imo, avoiding redundant local variables is for performance and readability, performance refers to every variable needed to create memory space, and readability refers to the **one-time** intermediate variables that do not bring benefits but add cognitive load.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] zhaoyongjie commented on a diff in pull request #21163: feat: apply Time Grain to X-Axis column
Posted by GitBox <gi...@apache.org>.
zhaoyongjie commented on code in PR #21163:
URL: https://github.com/apache/superset/pull/21163#discussion_r961494284
##########
superset-frontend/packages/superset-ui-chart-controls/src/sections/sections.tsx:
##########
@@ -38,6 +38,19 @@ export const legacyTimeseriesTime: ControlPanelSectionConfig = {
],
};
+export const echartsTimeseriesTime: ControlPanelSectionConfig = {
Review Comment:
alright, I will change it to `genericTime`, but it might be removed after move `granularity_sqla` and `time_range` to the `filter`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org