[Discover]chore: disable sorting on columns header for PPL and SQL #9263

Maosaic · 2025-01-23T18:30:47Z

Description

Disable sorting on columns header for PPL and SQL,

Issues Resolved

Currently sorting on Discover table columns header does not support PPL and SQL, disable it until we build sorting support for PPL and SQL.

Screenshot

Screen.Recording.2025-01-22.at.10.36.39.AM.mp4

Testing the changes

Changelog

chore: Disable sorting on Discover table columns header for PPL and SQL

Check List

All tests pass
- yarn test:jest
- yarn test:jest_integration
New functionality includes testing.
New functionality has been documented.
Update CHANGELOG.md
Commits are signed per the DCO using --signoff

Signed-off-by: Joey Liu <[email protected]>

codecov · 2025-01-23T18:46:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.68%. Comparing base (0be70a7) to head (9eef96e).
Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9263   +/-   ##
=======================================
  Coverage   61.68%   61.68%           
=======================================
  Files        3816     3816           
  Lines       91693    91695    +2     
  Branches    14516    14516           
=======================================
+ Hits        56557    56559    +2     
  Misses      31510    31510           
  Partials     3626     3626

Flag	Coverage Δ
Linux_1	`29.04% <0.00%> (-0.01%)`	⬇️
Linux_2	`56.46% <0.00%> (ø)`
Linux_3	`39.09% <100.00%> (?)`
Linux_4	`28.97% <0.00%> (-0.01%)`	⬇️
Windows_1	`29.06% <0.00%> (-0.01%)`	⬇️
Windows_2	`56.41% <0.00%> (ø)`
Windows_3	`39.09% <100.00%> (+<0.01%)`	⬆️
Windows_4	`28.97% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

angle943

I have tested this change manually and the functionality works as expected!

Just FYI i will update my cypress test PR that tests for language-specific display and update it to ensure that sort works as expected (so it is dependent upon your PR being merged): #9215

angle943 · 2025-01-24T00:42:06Z

src/plugins/discover/public/application/components/default_discover_table/helper.tsx

    return {
      name: column,
      displayName: isShortDots ? shortenDottedString(column) : column,
-      isSortable: field && field.sortable ? true : false,
+      isSortable:
+        osdFieldOverrides.sortable == null ? !!field?.sortable : osdFieldOverrides.sortable,


nitpick: is the !! necessary for !!field?.sortable? The combination of !! and ? (on top of the ternary) is hurting my brain

but on top of that i think nullish coalescing operator will help readability here

!!field?.sortable is the short handed version of previous code field && field.sortable ? true : false

i understand that, but isn't field?.sortable sufficent?

We need false if sortable is undefined

ah sortable is an optional field?

angle943 · 2025-01-24T16:49:03Z

src/plugins/data/common/osd_field_types/osd_field_types.ts

@@ -78,7 +78,7 @@ export const getFilterableOsdTypeNames = (): string[] =>
  registeredOsdTypes.filter((type) => type.filterable).map((type) => type.name);

 export const setOsdFieldOverrides = (newOverrides: { [key: string]: any } | undefined) => {
-  osdFieldOverrides = newOverrides ? Object.assign({}, osdFieldOverrides, newOverrides) : {};
+  osdFieldOverrides = newOverrides ? Object.assign({}, newOverrides) : {};
 };

 export const getOsdFieldOverrides = (): { [key: string]: any } => {


(unrelated to your PR) why is there osd_field_types.ts and old_field_type.ts in this directory?

angle943 · 2025-01-24T16:56:47Z

src/plugins/data/common/osd_field_types/osd_field_types.ts

@@ -78,7 +78,7 @@ export const getFilterableOsdTypeNames = (): string[] =>
  registeredOsdTypes.filter((type) => type.filterable).map((type) => type.name);

 export const setOsdFieldOverrides = (newOverrides: { [key: string]: any } | undefined) => {
-  osdFieldOverrides = newOverrides ? Object.assign({}, osdFieldOverrides, newOverrides) : {};


(clarifying comment). I'm trying to understand this change. So initially:

if newOverrides, then osdFieldOverrides would include both properties of osdFieldOverrides and newOverrides, else it would be empty object

With your change:

if newOverrides, it would just be newOverrides, else an empty object?

Yours make sense to me, but the original code doesn't make much sense. It would make more sense if on the else condition of the ternary, the osdFieldOverrides was getting applied, but the way it is written it looks like that is appearing out of nowhere

(but now more of a nitpick): Would it be better if we just did:

osdFieldOverrides = {...newOverrides}

?

it would cover the undefined case as well

I'm align with your suggestion, I was trying to introduce less change if possible, also want to have @kavilla align with the change here.

correct it made sense at one time for this code but this was essentially dead code.

angle943 · 2025-01-24T17:04:40Z

src/plugins/data/public/query/query_string/language_service/language_service.ts

-  DQLBody,
-  QueryEditorExtensionConfig,
-  SingleLineInput,
-  UiEnhancements,


(clarifying comment) did this UiEnhancements type never exist? I'm trying to find it in my IDE but can't find it :/

This is unrelated to the code change of disabling sorting, but a ts error I found when I touching the file.

I think this was supposed to be updated in #PR7731 , and this is why I think we should not bypass ts error when building

it did but the delta of changes involved in changing it close to cut off dates made it not worth it with a lot changing. so thanks for fixing it.

src/plugins/discover/public/application/components/default_discover_table/helper.tsx

Signed-off-by: Joey Liu <[email protected]>

kavilla · 2025-01-28T19:51:38Z

src/plugins/discover/public/application/components/default_discover_table/helper.tsx

@@ -83,12 +90,14 @@ export function getLegacyDisplayedColumns(
  if (!Array.isArray(columns) || typeof indexPattern !== 'object' || !indexPattern.getFieldByName) {
    return [];
  }
+  // TODO: Remove overrides once we support PPL/SQL sorting


did we want to create an issue and track it on github and link a reference in here?

kavilla · 2025-01-28T19:56:18Z

src/plugins/discover/public/application/components/default_discover_table/helper.test.tsx

@@ -5,8 +5,11 @@

 import { getLegacyDisplayedColumns } from './helper';
 import { IndexPattern } from '../../../opensearch_dashboards_services';
+import { getOsdFieldOverrides } from 'src/plugins/data/common/osd_field_types/osd_field_types';


did we want to import similiar to the actual file

import { getOsdFieldOverrides } from 'src/plugins/data/common';

kavilla

just some nits. but lgtm.

[Discover]chore: disable sorting on columns header for PPL and SQL

3649784

Signed-off-by: Joey Liu <[email protected]>

github-actions bot added the repeat-contributor label Jan 23, 2025

Changeset file for PR opensearch-project#9263 created/updated

3fb4b8c

ananzh added bug Something isn't working discover-next labels Jan 23, 2025

ananzh added the v2.19.0 label Jan 23, 2025

angle943 reviewed Jan 24, 2025

View reviewed changes

[Discover]chore: disable sorting on timestamp column for PPL and SQL

9eef96e

Signed-off-by: Joey Liu <[email protected]>

Maosaic force-pushed the sort branch from 81ced5f to 9eef96e Compare January 26, 2025 20:36

kavilla reviewed Jan 28, 2025

View reviewed changes

kavilla approved these changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover]chore: disable sorting on columns header for PPL and SQL #9263

[Discover]chore: disable sorting on columns header for PPL and SQL #9263

Maosaic commented Jan 23, 2025

codecov bot commented Jan 23, 2025 •

edited

Loading

angle943 left a comment •

edited

Loading

angle943 Jan 24, 2025

angle943 Jan 24, 2025

Maosaic Jan 24, 2025

angle943 Jan 24, 2025

Maosaic Jan 27, 2025

angle943 Jan 28, 2025

angle943 Jan 24, 2025

angle943 Jan 24, 2025

angle943 Jan 24, 2025

Maosaic Jan 24, 2025

kavilla Jan 28, 2025

angle943 Jan 24, 2025

Maosaic Jan 24, 2025

kavilla Jan 28, 2025

kavilla Jan 28, 2025

kavilla Jan 28, 2025

kavilla left a comment

[Discover]chore: disable sorting on columns header for PPL and SQL #9263

Are you sure you want to change the base?

[Discover]chore: disable sorting on columns header for PPL and SQL #9263

Conversation

Maosaic commented Jan 23, 2025

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

codecov bot commented Jan 23, 2025 • edited Loading

Codecov Report

angle943 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 23, 2025 •

edited

Loading

angle943 left a comment •

edited

Loading