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

Fix missing column totals in data table. #34169

Merged
merged 4 commits into from
Apr 3, 2019

Conversation

lukeelmers
Copy link
Member

@lukeelmers lukeelmers commented Mar 29, 2019

Fixes #34046
Regression seems to have been introduced in #28746

The data table relies on isNumeric and isDate params to be defined
on each dimension so that it can determine how to calculate and format
column totals when they are enabled.

These params were missing from the getSchemas utility, causing the
data table to render empty rows for totals.

We have tests to catch this, but they were using mocked data with the
correct params, while the actual code was generating dimensions
without those params.

This is another case we should consider when adding tests for
getSchemas in #33196.

The data table relies on `isNumeric` and `isDate` params to be defined
on each dimension so that it can determine how to calculate and format
column totals when they are enabled.

These params were missing from the `getSchemas` utility, causing the
data table to render empty rows for totals.

Fixes elastic#34046
@lukeelmers lukeelmers added Feature:Data Table Data table visualization feature Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) regression release_note:fix v7.0.0 v8.0.0 :AppArch v7.2.0 labels Mar 29, 2019
@lukeelmers lukeelmers requested a review from ppisljar March 29, 2019 16:56
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are adding isDate and isNumber to schema config but:

  • this seems to be more related to schema format ?
  • this seems to only be needed by table vis ?
  • we already have schema.params.date (and now introducing isDate)

@lukeelmers
Copy link
Member Author

Took a more minimal approach per discussion with @ppisljar... The isDate and isNumeric checks now just happen inside the agg_table directive, making them specific to table vis, as other visualizations should not need this info, and we are rewriting table vis eventually anyway.

Kept the getSchemas refactoring in place, but it introduces no functionality changes.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

totals don't work for terms aggregation on number field (not sure if that was ever a thing)

apart from that LGTM (tested in chrome linux)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@lukeelmers lukeelmers merged commit 5bb9947 into elastic:master Apr 3, 2019
@lukeelmers lukeelmers deleted the fix/data-table-total branch April 3, 2019 22:05
lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Apr 3, 2019
lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Apr 3, 2019
@lukeelmers
Copy link
Member Author

@tzachshabtay
Copy link

@lukeelmers We're using 7.1.1 and we don't see the total row, is the fix in that version?

@lukeelmers
Copy link
Member Author

@tzachshabtay Yes, the fix made it to the 7.1 branch so it should be working in 7.1.1

I just tested in 7.1.1, and it seems to be working fine for me: when using a table with split rows on a terms aggregation with the "show totals" option checked, I see the totals as expected.

What type of aggregation are you using where you aren't seeing the totals?

@tzachshabtay
Copy link

tzachshabtay commented Jun 27, 2019

@lukeelmers we upgraded to 7.2 but it's still not showing.
We're using the data table visualization, split rows on term aggregation as well.
We're using the sum as our "total function".

If we switch to count as our "total function" then it does show, but any other total function doesn't show (and it did show on kibana 6.x).

Some screenshots of our setup:

Screen Shot 2019-06-27 at 9 50 54 AM

Screen Shot 2019-06-27 at 9 51 14 AM

Here showing that it works with count:
Screen Shot 2019-06-27 at 9 51 23 AM

@tzachshabtay
Copy link

Seems to be fixed in version 7.3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Aggregations Aggregation infrastructure (AggConfig, esaggs, ...) Feature:Data Table Data table visualization feature regression release_note:fix v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show total does not render any value but count in datatable
4 participants