Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deleting columns doesn't delete affiliated column title #12679

Open
mlm603 opened this issue Jun 10, 2020 · 4 comments
Open

Deleting columns doesn't delete affiliated column title #12679

mlm603 opened this issue Jun 10, 2020 · 4 comments
Labels
.Backend .Correctness Difficulty:Medium Priority:P3 Cosmetic bugs, minor bugs with a clear workaround .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects Visualization/Chart Settings .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2

Comments

@mlm603
Copy link

mlm603 commented Jun 10, 2020

Describe the bug

When you have two of the same types of aggregations (e.g. Avg of Price and Avg of Rating), if you rename Avg of Price and then delete it, Avg of Rating takes on the custom name that was applied to Avg of Price.

Logs

Only console log that comes up during this process is

Warning: setState(...): Can only update a mounted or mounting component. This usually means you called setState() on an unmounted component. This is a no-op. Please check the code for the Draggable component.
r @ react_devtools_backend.js:6
printWarning @ warning.js:33
warning @ warning.js:57
getInternalInstanceReadyForUpdate @ ReactUpdateQueue.js:46
enqueueSetState @ ReactUpdateQueue.js:207
../../../node_modules/react/lib/ReactBaseClasses.js.ReactComponent.setState @ ReactBaseClasses.js:62
Draggable._this.onDragStop @ react-draggable.js:1972
DraggableCore._this.handleDragStop @ react-draggable.js:1650

On the server side:

[0e730563-d412-40eb-a39c-e9ef85537d44] 2020-06-10T18:29:21-04:00 DEBUG metabase.middleware.log POST /api/dataset 200 [ASYNC: completed] 26.6 ms (15 DB calls) (69 total active threads) Queries in flight: 0

To Reproduce
Steps to reproduce the behavior:

  1. Create a Question with two of the same type of aggregation. E.g. using the Sample Dataset's Products table, select avg of Price and avg of Rating and group by Vendor
  2. Change the column title of the furthest left aggregation (I did this using the Formatting window that pops up when you click on the column header)
  3. Delete the column that you just renamed. You will see that the custom column title is still there, but it's actually showing the values for the aggregation that used to be second from the left

Expected behavior

Custom titles should be tied to specific metrics, not to the position of a metric in the results. E.g. if I rename Sum of Price to Price! and delete it, the header labelled Price! should go away too.

Screenshots
Initial table
initial_table

Renamed Sum of Price -> Price!
renamed_column

Deleted Sum of Price
final

Information about your Metabase Installation:

  • Your browser and the version: Chrome 83.0.4103.61
  • Your operating system: OS X 10.14.6
  • Your databases: tested on Redshift and on Sample Data
  • Metabase version: tested on v1.35.0 and v0.35.0
  • Metabase hosting environment: tested on localhost (with Docker)
  • Metabase internal database: postgres

Severity

Since it's really a cosmetic issue, I think this is low severity. Many of our users don't bother to rename columns, anyway. It will mostly be annoying for the couple of Data Analysts on our team.

Additional context

  • This only seems to apply when it's two columns with the same type of aggregation. When one column was a sum and the other is an avg, it behaves as expected
  • I originally noticed this in a table viz, but it seems to apply to other viz types as well
  • just from poking around in the React elements a little bit, I suspect this has something to do with the columns being aliased like sum_1, sum_2, etc.
@mlm603 mlm603 added .Needs Triage Type:Bug Product defects labels Jun 10, 2020
@flamber flamber added Priority:P3 Cosmetic bugs, minor bugs with a clear workaround Visualization/Chart Settings and removed .Needs Triage labels Jun 11, 2020
@flamber
Copy link
Contributor

flamber commented Jun 11, 2020

This is a known problem with how visualization settings for columns are stored #11829.
Related to #12625

Reproduce:

  1. Custom question > Sample Dataset > Products
  2. Summarize "Sum of Price" and "Sum of Rating" grouped by Category
  3. Table visualization, rename the two sum columns, example ColPrice and ColRating
    image
  4. Now remove the "Sum of Price" metric - notice that the "Sum of Rating" is now called "ColPrice" incorrectly
    image

@luizarakaki luizarakaki added the .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2 label Jul 19, 2023
@deniskaber deniskaber self-assigned this Nov 8, 2023
@deniskaber
Copy link
Contributor

This issue can be reproduced on the current master (cac52d8).
It is a specific case of a known issue #11829 as @flamber already mentioned earlier in this issue.

@deniskaber deniskaber removed their assignment Nov 8, 2023
@darksciencebase darksciencebase added the .Team/QueryProcessor :hammer_and_wrench: label Nov 21, 2023
@camsaul
Copy link
Member

camsaul commented Feb 20, 2024

I don't think this is really an MLv2 thing at this point, it's related to Viz Settings which are not currently part of MLv2

@bshepherdson
Copy link
Contributor

However, this is related to field refs and so will be affected by steps taken to fix #36185.

Refs are currently subject to a kind of "late binding capture" like unhygienic Lisp macros, and this is one example.

It can be done inside MLv2 as well:

  • Orders with Sums of Subtotal, Tax and Total; broken out by week.
    • Visualize: 209 rows
  • Add a filter on the second one, Sum of Tax > 100.
    • Visualize: now shows 174 of 209 rows
  • Delete the first aggregation (Sum of Subtotal)
    • The filter is now aimed at Sum of Total (previously third, now second) - oops!
  • The results track that new reality: 208 of 209 rows.

Of course the trouble here is that the filters are using refs by name: sum, sum_2 and sum_3. So the filter is [:> [:field "sum_2" nil] 100] and that's only "late bound" to the aggregation. metabase.lib.remove-replace correctly deletes refs to the deleted entity, but it doesn't spot this issue. (I think remove-replace is the wrong level to be trying to fix this.)

Exploring all the permutations:

Filtering on Delete Result Evaluation
1 1 Filter deleted ✔️
1 2 Filter preserved ✔️
1 3 Filter preserved ✔️
2 1 Filter retargets to last sum (the above case)
2 2 Filter deleted ✔️
2 3 Filter preserved ✔️
3 1 Filter broken! Sum 3 is greater than 2000
3 2 Filter broken! Same as 3/1
3 3 Filter deleted ✔️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Backend .Correctness Difficulty:Medium Priority:P3 Cosmetic bugs, minor bugs with a clear workaround .Team/QueryProcessor :hammer_and_wrench: Type:Bug Product defects Visualization/Chart Settings .Wanted: MLv2 Issues that will be fixed (or easier to fix, or possible to fix) when we have MLv2
Projects
None yet
Development

No branches or pull requests

9 participants