feat(explore): display metrics and columns in alphabetical order #33140
Conversation
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Inconsistent null value representation ▹ view | 🧠 Not in standard |
|
Duplicate sorting logic ▹ view | 🧠 Incorrect |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/src/explore/components/DatasourcePanel/types.ts | ✅ |
| superset-frontend/src/explore/components/DatasourcePanel/transformDatasourceFolders.ts | ✅ |
| superset-frontend/src/explore/components/DatasourcePanel/fixtures.tsx | ✅ |
| superset-frontend/src/explore/components/DatasourcePanel/index.tsx | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Need a new review? Comment
/korbit-reviewon this PR and I'll review your latest changes.Korbit Guide: Usage and Customization
Interacting with Korbit
- You can manually ask Korbit to review your PR using the
/korbit-reviewcommand in a comment at the root of your PR.- You can ask Korbit to generate a new PR description using the
/korbit-generate-pr-descriptioncommand in any comment on your PR.- Too many Korbit comments? I can resolve all my comment threads if you use the
/korbit-resolvecommand in any comment on your PR.- On any given comment that Korbit raises on your pull request, you can have a discussion with Korbit by replying to the comment.
- Help train Korbit to improve your reviews by giving a 👍 or 👎 on the comments Korbit posts.
Customizing Korbit
- Check out our docs on how you can make Korbit work best for you and your team.
- Customize Korbit for your organization through the Korbit Console.
Current Korbit Configuration
General Settings
Setting Value Review Schedule Automatic excluding drafts Max Issue Count 10 Automatic PR Descriptions ❌ Issue Categories
Category Enabled Documentation ✅ Logging ✅ Error Handling ✅ Readability ✅ Design ✅ Performance ✅ Security ✅ Functionality ✅ Feedback and Support
superset-frontend/src/explore/components/DatasourcePanel/fixtures.tsx
Outdated
Show resolved
Hide resolved
| const sortMetrics = (arr: MetricItem[]) => | ||
| arr.sort((a, b) => { | ||
| const certCmp = Number(b?.is_certified ?? 0) - Number(a?.is_certified ?? 0); | ||
| if (certCmp) return certCmp; | ||
|
|
||
| const aName = (a.verbose_name || a.metric_name) ?? ''; | ||
| const bName = (b.verbose_name || b.metric_name) ?? ''; | ||
| return aName.localeCompare(bName, undefined, { sensitivity: 'base' }); | ||
| }); | ||
|
|
||
| const sortColumns = (arr: ColumnItem[]) => | ||
| arr.sort((a, b) => { | ||
| const certCmp = Number(b?.is_certified ?? 0) - Number(a?.is_certified ?? 0); | ||
| if (certCmp) return certCmp; | ||
|
|
||
| const aName = (a.verbose_name || a.column_name) ?? ''; | ||
| const bName = (b.verbose_name || b.column_name) ?? ''; | ||
| return aName.localeCompare(bName, undefined, { sensitivity: 'base' }); | ||
| }); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
This might be a problem for certain charts like the Table where you want these in a particular order to arrange columns. |
vanaigr
force-pushed
the
explore-sort-alphabetical
branch
from
5847993 to
b51ce09
Compare
I can add a switch at the top of the panel that changes the order between this and old one, or a configuration parameter. Datetime columns are shown out of order, so the order doesn't match that of the source table. Since the client doesn't sort columns beyond that, I tried to see which order the columns are returned from the server. But the query doesn't have an Queries triggered by SELECT
table_columns.uuid AS table_columns_uuid, table_columns.created_on AS table_columns_created_on, table_columns.changed_on AS table_columns_changed_on,
table_columns.id AS table_columns_id, table_columns.column_name AS table_columns_column_name,
table_columns.verbose_name AS table_columns_verbose_name, table_columns.is_active AS table_columns_is_active,
table_columns.type AS table_columns_type, table_columns.advanced_data_type AS table_columns_advanced_data_type,
table_columns.groupby AS table_columns_groupby, table_columns.filterable AS table_columns_filterable,
table_columns.description AS table_columns_description, table_columns.table_id AS table_columns_table_id,
table_columns.is_dttm AS table_columns_is_dttm, table_columns.expression AS table_columns_expression,
table_columns.python_date_format AS table_columns_python_date_format, table_columns.extra AS table_columns_extra,
table_columns.created_by_fk AS table_columns_created_by_fk, table_columns.changed_by_fk AS table_columns_changed_by_fk
FROM table_columns
WHERE ? = table_columns.table_idand the same applies to metrics: SELECT
sql_metrics.uuid AS sql_metrics_uuid, sql_metrics.created_on AS sql_metrics_created_on, sql_metrics.changed_on AS sql_metrics_changed_on,
sql_metrics.id AS sql_metrics_id, sql_metrics.metric_name AS sql_metrics_metric_name, sql_metrics.verbose_name AS sql_metrics_verbose_name,
sql_metrics.metric_type AS sql_metrics_metric_type, sql_metrics.description AS sql_metrics_description, sql_metrics.d3format AS sql_metrics_d3format,
sql_metrics.currency AS sql_metrics_currency, sql_metrics.warning_text AS sql_metrics_warning_text, sql_metrics.table_id AS sql_metrics_table_id,
sql_metrics.expression AS sql_metrics_expression, sql_metrics.extra AS sql_metrics_extra, sql_metrics.created_by_fk AS sql_metrics_created_by_fk,
sql_metrics.changed_by_fk AS sql_metrics_changed_by_fk
FROM sql_metrics
WHERE ? = sql_metrics.table_idUPD: Request is hadled by But I couldn't find sorting on this path. |
@rusackas I believe that would be a problem only if this actually changes the sorting of the chart config (the order of selected dimensions, metrics, columns). If it just organizes these under the Chart Source left menu I believe it might not be a concern? As long as charts aren't updated... I have the impression that currently temporal columns are always on top -- not sure if that's something we want to maintain or not? I wonder if that's the case because in the past most charts were only temporal. |
SUMMARY
Changed the order in which metrics and columns are displayed in the dataset panel on
/explorepage.Before, columns were sorted by whether they are certified and whether they are datetime. Metrics were sorted by certification (but only when searching?).
Now, both columns and metrics are sorted by certification, then alphabetically, case and accent insensitively. This doesn't apply to folders, since I assume the feature is in progress.
Rationale:
Some users don't find the current order natural, especially in tables with many columns. I checked and it seems that the order of columns (before sorting for display) is different from that in the source table.
I assume there should be a setting or a config flag that changes the order to the old one, since there are benefits in showing datetime columns first? Where should I add it?
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Left image is before, right is after:
TESTING INSTRUCTIONS
Open a chart, e.g. "wb_health_population" "Box plot" from the example data.
Metrics should be displayed in alphabetical order, and columns should be in alphabetical order, but all certified metrics and columns should be displayed before uncertified.
Uppercase letters should not matter.
Display name should be used, which may be different from column/metric name.
When searching, results should be sorted the same way.
ADDITIONAL INFORMATION