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/12/19 11:19:37 UTC
[GitHub] [superset] Always-prog opened a new pull request, #22453: Fix(Annotation Layers): Error when render options with renamed columns
Always-prog opened a new pull request, #22453:
URL: https://github.com/apache/superset/pull/22453
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
- User got an error if he trying to use renamed columns for the Annotation Layers. This is fix for it by getMetricName function.
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
#### Before
#### After
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Create a interval table with two SQL Expression columns `'1970-02-16 17:45+00'::timestamp` and `'1990-02-16 17:45+00'::timestamp`, name it `start` and `end`.
2. Go to Birth Names Line chart, and try to add interval annotation layer from created table in the step 1 table.
3. You will see an error because renamed columns was parsed incorrect.
### 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] Always-prog commented on pull request #22453: fix(Annotation Layers): Error when render options with renamed columns
Posted by GitBox <gi...@apache.org>.
Always-prog commented on PR #22453:
URL: https://github.com/apache/superset/pull/22453#issuecomment-1384190304
@villebro Hello!
I checked your code for working about month ago and re-request review.
Can you please review my changes again?
--
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 #22453: Fix(Annotation Layers): Error when render options with renamed columns
Posted by GitBox <gi...@apache.org>.
villebro commented on code in PR #22453:
URL: https://github.com/apache/superset/pull/22453#discussion_r1052482575
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx:
##########
@@ -28,6 +28,7 @@ import {
validateNonEmpty,
isValidExpression,
styled,
+ getMetricLabel,
Review Comment:
```suggestion
getColumnLabel,
```
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx:
##########
@@ -326,7 +327,19 @@ class AnnotationLayer extends React.PureComponent {
metadata && metadata.canBeAnnotationType(annotationType)
);
})
- .map(x => ({ value: x.id, label: x.title, slice: x })),
+ .map(x => ({
+ value: x.id,
+ label: x.title,
+ slice: {
+ ...x,
+ data: {
+ ...x.data,
+ groupby: x.data.groupby.map(metricOrColumn =>
+ getMetricLabel(metricOrColumn),
+ ),
Review Comment:
I know this is can be a pain to grasp from the JS codebase, but the type of `groupby` is actually `QueryFormColumn`, and the correct label getter is `getColumnLabel`:
```suggestion
groupby: x.data.groupby.map(column =>
getColumnLabel(column),
),
```
--
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] Always-prog commented on a diff in pull request #22453: fix(Annotation Layers): Error when render options with renamed columns
Posted by GitBox <gi...@apache.org>.
Always-prog commented on code in PR #22453:
URL: https://github.com/apache/superset/pull/22453#discussion_r1052589082
##########
superset-frontend/src/explore/components/controls/AnnotationLayerControl/AnnotationLayer.jsx:
##########
@@ -326,7 +327,19 @@ class AnnotationLayer extends React.PureComponent {
metadata && metadata.canBeAnnotationType(annotationType)
);
})
- .map(x => ({ value: x.id, label: x.title, slice: x })),
+ .map(x => ({
+ value: x.id,
+ label: x.title,
+ slice: {
+ ...x,
+ data: {
+ ...x.data,
+ groupby: x.data.groupby.map(metricOrColumn =>
+ getMetricLabel(metricOrColumn),
+ ),
Review Comment:
That code works fine.
I changed function for getting label in the last commit, thanks!
--
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 merged pull request #22453: fix(Annotation Layers): Error when render options with renamed columns
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro merged PR #22453:
URL: https://github.com/apache/superset/pull/22453
--
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