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

Change: add support for filtered data in Pivot table visualization #1666

Merged
merged 3 commits into from
Mar 26, 2017

Conversation

deecay
Copy link
Contributor

@deecay deecay commented Mar 10, 2017

This PR changes how the data fed to pivottable visualization is prepared, and makes pivottable show filtered and formatted data just as you can see in Table visualization.

I have noticed that pivottable visualization:

  • does not incorporate query/dashboard filter
  • ::filter annotation appears as part of pivottable attribute name and does not get removed
  • date and datetime columns does not respect REDASH_DATE_FORMAT

This PR addresses all three points.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Thanks! This is great, but see my comment about formatting the values.

const rowObj = {};
columns.forEach((col) => {
const colName = getColumnCleanName(col.name);
const value = formatValue($filter, clientConfig, row[col.name], col.type);
Copy link
Member

Choose a reason for hiding this comment

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

Two things that come to mind here:

  1. Are we sure that converting numbers into strings will behave properly with PivotTable?
  2. For dates, shouldn't we use some standard like (YYYY-MM-DD) regardless of configuration to solve Time series in pivot tables appear to be ordered by first letter of month - rather than by date #216?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • Comment 2

    I am thinking of adding a Sorting option to pivottable editor. Because passing a sort-friendly format will work for numerical dates, but it doesn't help for Monday/Tuesday/..., Mon/Tue/..., January/February/..., Jan/Feb/..., etc.
    If we pass REDAH_DATE_FORMAT to pivot, the problem below will go away because the dates won't be in ISO format,

and letting people define custom sorters will take care of the first comment of #216.

  • Comment 1
    I have to look into it a bit more for part 1 of your comment.

Copy link
Member

Choose a reason for hiding this comment

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

They have a way to pass a sort function?

Anyway, how about we change this pull request to only handle the filters part and leave the formatting to another pull request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes they do. Example is here.
I have to come up with how to let people define it in pivottable editor, but currently have no idea.

I'm perfectly fine with dividing this PR into two, and making this only about the filters.
I assume what you mean is just replacing .getRawData() to .getData()?

Copy link
Member

Choose a reason for hiding this comment

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

I have to come up with how to let people define it in pivottable editor, but currently have no idea.

The user shouldn't have to define them. We should have default ones for the types we have (dates, numbers, etc - where needed).

@deecay deecay changed the title Pivottable should show filtered and formatted data Pivottable should show filtered data Mar 23, 2017
@deecay
Copy link
Contributor Author

deecay commented Mar 23, 2017

This PR has been changed and now it is about fixing pivottable of filtered data.

Formatting part will be addressed in a separate PR.

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

Successfully merging this pull request may close these issues.

None yet

2 participants