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

Support injecting DataStructureMeta from QueryEditorExtensions for Query Assist #7871

Merged
merged 5 commits into from
Aug 28, 2024

Conversation

joshuali925
Copy link
Member

@joshuali925 joshuali925 commented Aug 27, 2024

Description

  • fix query editor extensions enhance key
  • add query assist icons to clusters if available
  • allow custom icon for DATA_STRUCTURE_META_TYPES.FEATURE

TODO items:

  1. replace the query assist icon. @canascar if we have a new icon, do you have the svg file for it? and is there a preferred tooltip for query assist?
  2. confirm usage of DataStructureMeta @ashwin-pc @kavilla
    /**
    * Metadata for a data structure, used for additional properties like icons or tooltips.
    */
    export interface DataStructureFeatureMeta {
    type: DATA_STRUCTURE_META_TYPES.FEATURE;
    icon?: string;
    tooltip?: string;
    }
    /**
    * Metadata for dataset type
    */
    export interface DataStructureDataTypeMeta {
    type: DATA_STRUCTURE_META_TYPES.TYPE;
    icon: EuiIconProps;
    tooltip: string;
    }
    /**
    * Metadata for a data structure with CUSTOM type, allowing any additional fields.
    */
    export interface DataStructureCustomMeta {
    type: DATA_STRUCTURE_META_TYPES.CUSTOM;
    [key: string]: any;
    }
    1. To use custom icons, I changed type to TYPE, which should be "metadata for dataset type". But for extensions I think FEATURE would make more sense. Is it on purpose to only support built-in OUI icons for FEATURE?
    2. Currently index patterns have type CUSTOM, is it intentional to not support icons for it?

Issues Resolved

Screenshot

image

Testing the changes

Changelog

  • feat: Support injecting DataStructureMeta from QueryEditorExtensions for Query Assist

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: Joshua Li <joshuali925@gmail.com>
Signed-off-by: Joshua Li <joshuali925@gmail.com>
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

Attention: Patch coverage is 13.15789% with 33 lines in your changes missing coverage. Please review.

Project coverage is 64.21%. Comparing base (930e244) to head (a249227).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...ic/query/query_string/dataset_service/lib/utils.ts 6.25% 15 Missing ⚠️
...nts/public/query_assist/utils/create_extension.tsx 22.22% 14 Missing ⚠️
...y_string/dataset_service/lib/index_pattern_type.ts 0.00% 2 Missing ⚠️
...ery/query_string/dataset_service/lib/index_type.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7871      +/-   ##
==========================================
- Coverage   64.22%   64.21%   -0.02%     
==========================================
  Files        3672     3673       +1     
  Lines       81152    81175      +23     
  Branches    12938    12940       +2     
==========================================
+ Hits        52121    52127       +6     
- Misses      25822    25840      +18     
+ Partials     3209     3208       -1     
Flag Coverage Δ
Linux_1 30.20% <5.00%> (-0.01%) ⬇️
Linux_2 56.08% <ø> (ø)
Linux_3 40.55% <5.00%> (+<0.01%) ⬆️
Linux_4 31.31% <13.15%> (-0.01%) ⬇️
Windows_1 30.21% <5.00%> (-0.01%) ⬇️
Windows_2 56.03% <ø> (ø)
Windows_3 40.55% <5.00%> (-0.02%) ⬇️
Windows_4 31.31% <13.15%> (-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.

@joshuali925 joshuali925 marked this pull request as draft August 28, 2024 00:30
Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925 joshuali925 marked this pull request as ready for review August 28, 2024 01:23
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

The code looks good. Looks like you have a failing unit test though.

* @returns list of query assist supported languages for the given data source.
*/
const getAvailableLanguagesForDataSource = (() => {
const availableLanguagesByDataSource: Map<string | undefined, string[]> = new Map();
Copy link
Member

Choose a reason for hiding this comment

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

Nice way to cache the data

Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker but can we add some tests for this function?

* @param selectDataSourceId - function to get data source id given a data structure
* @returns data structures with meta
*/
export const injectMetaToDataStructures = async (
Copy link
Member

Choose a reason for hiding this comment

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

@ashwin-pc are we sure we don't want to do convert the dataset config to a class.

I was thinking it would be similar to the the interceptor class:

https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/src/plugins/data/public/search/search_interceptor.ts#L57

This way we don't have to ensure developers import this in their configs.

@joshuali925 could implement this in the base class and then it would be available for classes that extend the base class

Copy link
Member Author

Choose a reason for hiding this comment

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

agree. will think about it later, right now just trying to add instead of refactor..

@@ -95,7 +95,7 @@ export class QueryEnhancementsPlugin
queryString.getLanguageService().registerLanguage(sqlLanguageConfig);

data.__enhance({
ui: {
editor: {
Copy link
Member

Choose a reason for hiding this comment

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

sorry about that. Thanks for fixing.

@joshuali925 joshuali925 merged commit 9bc2433 into opensearch-project:main Aug 28, 2024
128 of 130 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 28, 2024
…r Query Assist (#7871)

* fix query editor extensions enhance key

Signed-off-by: Joshua Li <joshuali925@gmail.com>

* add query assist icons to clusters if available

Signed-off-by: Joshua Li <joshuali925@gmail.com>

* allow custom icon for `DATA_STRUCTURE_META_TYPES.FEATURE`

Signed-off-by: Joshua Li <joshuali925@gmail.com>

* Changeset file for PR #7871 created/updated

---------

Signed-off-by: Joshua Li <joshuali925@gmail.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>
(cherry picked from commit 9bc2433)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
ashwin-pc added a commit that referenced this pull request Aug 28, 2024
…r Query Assist (#7871) (#7888)

* fix query editor extensions enhance key



* add query assist icons to clusters if available



* allow custom icon for `DATA_STRUCTURE_META_TYPES.FEATURE`



* Changeset file for PR #7871 created/updated

---------




(cherry picked from commit 9bc2433)

Signed-off-by: Joshua Li <joshuali925@gmail.com>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>
Co-authored-by: Ashwin P Chandran <ashwinpc@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants