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

[SIP-123] Proposal replacement of data table components with ag-grid #27645

Closed
justinpark opened this issue Mar 25, 2024 · 15 comments
Closed

[SIP-123] Proposal replacement of data table components with ag-grid #27645

justinpark opened this issue Mar 25, 2024 · 15 comments
Assignees
Labels
sip Superset Improvement Proposal

Comments

@justinpark
Copy link
Member

justinpark commented Mar 25, 2024

[SIP-123] Proposal replacement of data table components with ag-grid

Motivation

The data table, similar to a query result in SQL LAB, has been extensively utilized for data exploration. While the current ant-table has been a satisfactory solution, it has limitations when it comes to supporting advanced features.
Users have expressed their desire for more powerful functionalities such as "resizing a column width," "pinning/unpinning a column," "hiding/unhiding a column," and "multiple column sorting." Although there is a demo version available in ant-table that supports some of these features, it is still premature and buggy, and no longer actively maintained.
Fortunately, there are several alternative open-source solutions that already support these features, making replacement a suitable approach in the current situation.

Proposed Change

After thorough benchmarking of various open-source solutions, ag-grid has emerged as the best choice due to its extensive APIs and support for the advanced features we require (as mentioned above). Additionally, it is fully MIT licensed and has a separate enterprise version, making it a reliable option for future maintenance.
At Airbnb, we have successfully built a data table using the community version of ag-Grid and have utilized its open APIs to enable all the desired advanced features. It has been working well for us since this year. Today, I am excited to share this solution with the open-source community.

Image

New or Changed Public Interfaces

There are no interface changes. We are simply remapping the existing properties to the new ag-Grid component.

New Dependencies

ag-grid-community / ag-grid-react

Migration Plan and Compatibility

The migration process starts with the Table component in FilterableTable, which is used in the SQL Lab query result. Following that, we will migrate the TableCollection used in explore/chart result preview, and then the Table components(i.e. Pivot Table, DataTable) in chart-plugins.

As ag-theme-quartz comes recommended, we will be customizing the style based on the quartz theme.

Since Ag-grid conveniently provides CSS variables for custom styling, we can easily override these variables to achieve the desired style. To apply the quartz theme, all ag-grid tables will be enclosed within the .ag-theme-quartz class name. This will allow us to easily customize the style to match the existing theme style. For example, let's take a look at how we can match the font-family as an example.

 <Global
    styles={({ theme }) => css`
        .ag-theme-quartz {
          --ag-font-family: ${theme.typography.families.sansSerif};
        }
    `}
 />

Rejected Alternatives

None

@justinpark justinpark added the sip Superset Improvement Proposal label Mar 25, 2024
@justinpark
Copy link
Member Author

cc: #24318

@michael-s-molina michael-s-molina changed the title Proposal replacement of data table components with ag-grid [SIP] Proposal replacement of data table components with ag-grid Mar 26, 2024
@Hokwang
Copy link

Hokwang commented Mar 28, 2024

cc: #24319

@dacopan
Copy link

dacopan commented Mar 30, 2024

@justinpark also consider in the DataTable in chart-plugins the option to custom render of column for example we want present a list of tweets in a table with a column that renders to a hyperlink to navigate to a original tweet in twitter site.
I would like to contribute to the change of DataTable in chart-plugins

@rusackas
Copy link
Member

rusackas commented Apr 2, 2024

I've sent an email to the AG Grid team just to confirm that there won't be any licensing/EULA issues to contend with in our project, which might be used for commercial or non-commercial purposes. This seems like the occasion to ask for permission rather than forgiveness ;)

Also, the SIP makes no mention of converting the Table viz plugin to AG Grid, but assume that's a logical next step in the table roadmap if this phase goes according to plan?

@justinpark
Copy link
Member Author

the SIP makes no mention of converting the Table viz plugin to AG Grid, but assume that's a logical next step in the table roadmap if this phase goes according to plan?

That's right.

@justinpark
Copy link
Member Author

we want present a list of tweets in a table with a column that renders to a hyperlink to navigate to a original tweet in twitter site.
I would like to contribute to the change of DataTable in chart-plugins

Your contribution to this change would be highly appreciated, and it can be pursued independently of the dependencies mentioned in the proposal. This is because this proposal involves overriding the existing column rendering logic, allowing you to implement the desired hyperlink functionality separately.

@rusackas rusackas changed the title [SIP] Proposal replacement of data table components with ag-grid [SIP-123] Proposal replacement of data table components with ag-grid Apr 3, 2024
@michael-s-molina
Copy link
Member

I've sent an email to the AG Grid team just to confirm that there won't be any licensing/EULA issues to contend with in our project, which might be used for commercial or non-commercial purposes. This seems like the occasion to ask for permission rather than forgiveness ;)

Much appreciated @rusackas ❤️

Also, the SIP makes no mention of converting the Table viz plugin to AG Grid, but assume that's a logical next step in the table roadmap if this phase goes according to plan?

@justinpark I think it's important to add this to the SIP so folks that are not participating in the discussion can understand what's the plan. This depends on the reply of the Ag Grid folks and I can see two scenarios:

  • Orgs are not allowed to use the community version of the table when commercializing Superset. In this scenario, all tables still default to AgGrid but we need to add a configuration to disable it and show the old table instead.
  • No license restriction. We replace all tables and remove the old table.

During the SIP Office Hours, @yousoph also asked if there is any feature present in the current table that would not be available in AG Grid.

All this content could go under the Migration Plan and Compatibility section. We also need to add an example of how our theme could be applicable to the table to ensure visual consistency.

@dacopan
Copy link

dacopan commented Apr 4, 2024

Your contribution to this change would be highly appreciated

It would be a pleasure to contribute, could you give me a guide on the structure of the project to familiarize myself with the repository, if that is possible,
Is this the main thing to change?

https://github.com/apache/superset/tree/master/superset-frontend/plugins/plugin-chart-pivot-table

@dacopan
Copy link

dacopan commented Apr 4, 2024

I've sent an email to the AG Grid team just to confirm that there won't be any licensing/EULA issues to contend with in our project, which might be used for commercial or non-commercial purposes. This seems like the occasion to ask for permission rather than forgiveness ;)

according to repo of ag-grid has a MIT license https://github.com/ag-grid/ag-grid?tab=MIT-1-ov-file#readme

@justinpark
Copy link
Member Author

I think it's important to add this to the SIP so folks that are not participating in the discussion can understand what's the plan. This depends on the reply of the Ag Grid folks and I can see two scenarios:

  • Orgs are not allowed to use the community version of the table when commercializing Superset. In this scenario, all tables still default to AgGrid but we need to add a configuration to disable it and show the old table instead.
  • No license restriction. We replace all tables and remove the old table.

We've also received confirmation from the ag-Grid team regarding the use of the community version in Superset. In light of this, this proposal suggests replacing all tables with ag-Grid.

@michael-s-molina
Copy link
Member

michael-s-molina commented Apr 10, 2024

We've also received confirmation from the ag-Grid team regarding the use of the community version in Superset. In light of this, this proposal suggests replacing all tables with ag-Grid.

Wooohoooo! 🎉

@rusackas
Copy link
Member

We've also received confirmation from the ag-Grid team regarding the use of the community version in Superset.

Indeed. It looks like I forgot to mention that here, so thank you. Specifically, they stated in an email:

AG Grid community is free to download and use without the need to notify us.

As long as you are not using any of the features of AG Grid enterprise these will be no issue.

@aikawa-ohno
Copy link
Contributor

Will #26364 column filter like the one in a be implemented?

@justinpark
Copy link
Member Author

Will #26364 column filter like the one in a be implemented?

It can be a good idea for a follow-up improvement, but it is not included in the initial implementation. The implementation of a column filter like the one in #26364 would require further consideration and planning.

@rusackas
Copy link
Member

Closing since the vote passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sip Superset Improvement Proposal
Projects
Development

No branches or pull requests

9 participants