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

[BUG] Data Table Visualisation - The default limit of 3 "Split rows" aggregated buckets isn't applied #1178

Open
RoyiSitbon opened this issue Jan 25, 2022 · 6 comments
Labels
enhancement New feature or request

Comments

@RoyiSitbon
Copy link
Contributor

RoyiSitbon commented Jan 25, 2022

Describe the bug
The default limit of 3 “Split rows” aggregated buckets isn't applied on this visualization which allows adding more buckets and can harm cluster performance.

In other types of visualizations, there is a default limit of up to 3 “Split rows” aggregated buckets, e.g. Horizontal-Bar visualizations.

To Reproduce
Steps to reproduce the behavior:

  1. Click on Visualize
  2. Click on Create visualization
  3. Choose "Data table"
  4. Add more than 3 "Split rows" buckets.

Expected behavior
We wish to have an option to configure the max,min limitation for the “Split rows” aggregated bucket under dataTable vis.
i.e. Using this configuration, we will limit the max amount to 3:

Screen Shot 2022-02-03 at 14 35 48

Screenshots
Bad behavior -
splitRowsUnlimited

Good behavior -
splitRowsLimited

@RoyiSitbon RoyiSitbon added bug Something isn't working untriaged labels Jan 25, 2022
@ahopp ahopp added enhancement New feature or request and removed bug Something isn't working untriaged labels Jan 25, 2022
@kavilla kavilla linked a pull request Jan 26, 2022 that will close this issue
@jgough
Copy link
Contributor

jgough commented Feb 1, 2022

I thought that the limit on other visualisation types was not necessarily due to performance but more because adding more than 3 buckets on a line or bar graph makes little sense and would produce a very confusing visualisation. Is there something somewhere that says that the limit is because of performance?

@ananzh
Copy link
Member

ananzh commented Feb 25, 2022

Hey @RoyiSitbon I see we have a default advanced setting to set a default limit on the split row. Do we want to set a default limit or should we provide cx an option to set a limit? By creating a default limit, I think it will affect other customers experience because they might not want a limit on the split rows. It might also create some backward compatibility issue.

Meanwhile, when I play with the code, I found that even with a default limit to 3, I am still able to add more splits. At least, it is not grey which would then block me add more splits.
Screen Shot 2022-02-23 at 3 51 37 PM

@RoyiSitbon
Copy link
Contributor Author

Hey @RoyiSitbon I see we have a default advanced setting to set a default limit on the split row. Do we want to set a default limit or should we provide cx an option to set a limit? By creating a default limit, I think it will affect other customers experience because they might not want a limit on the split rows. It might also create some backward compatibility issue.

Meanwhile, when I play with the code, I found that even with a default limit to 3, I am still able to add more splits. At least, it is not grey which would then block me add more splits. Screen Shot 2022-02-23 at 3 51 37 PM

Hey, thanks a lot for replying. I agree and changed the behavior as asked.
Also regarding your check, thanks again. Fixed the bad behaviour to work as expected.

@ashwin-pc
Copy link
Member

ashwin-pc commented Jun 3, 2022

Hey @RoyiSitbon I had a look at #1179 and it looks like you are specifically targeting the table_viz. But this issue is more generic where we want to be able to customize the agg limits for the different aggregations for the visualizations. The thing is, every viz can optionally specify min and max agg values per agg schema. If no limit is specified in the vis
_type schema, it defaults to no limit.

We need to come up with a solution that respects the viz types limits and this feature should probably only restrict that limit further. Either at a global level or at a viz type/agg level. But im open to other suggestions.

Also the issue title seems to be misleading. I dont see a limit set by the split_row property in the table viz's type definitions schema

@ahopp
Copy link
Contributor

ahopp commented Jun 23, 2022

@ashwin-pc do you have any ideas on how we can generalizable solution that we can extend? If not, what would it cost us to push #1799 while we work on a solution that respects viz type or is more global? Seems like @RoyiSitbon has already done the table_viz work but I don't know how much debt or rework this would cause in the future...if the answer is "not much" if might be worth merging the local solution and iterating later on the global solution.

@RoyiSitbon any thoughts on above? I know this is holding #1179 and, selfishly, I would like to see this in the next release.

@ashwin-pc
Copy link
Member

@ahopp @RoyiSitbon I have some questions about the ask here before i can recommend some suggestions. Is the ask:

  1. Allow the user to set aggregation limits after the application has started up?
  2. Allow customizing the limits based on preferences and not actual technical limits.
  3. Add limits to the Data Table Viz similar to other viz types?

for context, this is how limits currently work in Visualize. Each viz type has a type definition which also defined the min and max fields that each visualization type supports. The Data Table viz type definition does not have this limit for the split rows aggregation, but does have some limits for the other aggregations. To add limits, we need to add them here and they will be respected. These limits however are limits of the agg/viz type and not user preferences.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants