[Discover] Modify the search bar info box content for sql/ppl #8708

RyanL1997 · 2024-10-26T07:05:16Z

Description

Modify the search bar info box content for sql/ppl
The info box will only show up when it is user's first time to select SQL/PPL
Change the language picker's title from "SQL" into "OpenSearch SQL"

Issues Resolved

Relate [Discover] notify user about field_type_tolerance for SQL/PPL in query footer #8702
we can close the above since this PR reflects the new design from UIUX

Screenshot

Version Final:

Screen.Recording.2024-10-27.at.2.26.53.PM.mov

Testing the changes

Changelog

fix: [Discover] Modify the search bar info box content for sql/ppl [Discover] Modify the search bar info box content for sql/ppl #8708

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: Ryan Liang <[email protected]>

github-actions · 2024-10-26T07:05:48Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

codecov · 2024-10-26T07:19:50Z

Codecov Report

Attention: Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.76%. Comparing base (463a6c0) to head (33b1ed2).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
...string/language_service/lib/language_reference.tsx	88.88%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8708   +/-   ##
=======================================
  Coverage   60.75%   60.76%           
=======================================
  Files        3798     3798           
  Lines       90650    90657    +7     
  Branches    14254    14257    +3     
=======================================
+ Hits        55076    55084    +8     
+ Misses      32079    32077    -2     
- Partials     3495     3496    +1

Flag	Coverage Δ
Linux_1	`29.06% <11.11%> (-0.01%)`	⬇️
Linux_2	`56.39% <ø> (ø)`
Linux_3	`37.58% <88.88%> (+0.01%)`	⬆️
Linux_4	`29.83% <11.11%> (-0.01%)`	⬇️
Windows_1	`29.09% <11.11%> (-0.01%)`	⬇️
Windows_2	`56.35% <ø> (ø)`
Windows_3	`37.58% <88.88%> (+<0.01%)`	⬆️
Windows_4	`29.83% <11.11%> (-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.

Signed-off-by: Ryan Liang <[email protected]>

Signed-off-by: Jialiang Liang <[email protected]>

github-actions · 2024-10-26T11:21:38Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Jialiang Liang <[email protected]>

github-actions · 2024-10-27T00:12:47Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

RyanL1997 · 2024-10-27T00:12:50Z

Unit testing is WIP. I'd gather some feedback first

github-actions · 2024-10-27T00:17:35Z

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc · 2024-10-27T01:14:56Z

src/plugins/data/public/query/query_string/language_service/lib/language_reference.tsx

+  useEffect(() => {
+    if (
+      props.selectedLanguage === 'SQL' &&
+      window.localStorage.getItem('hasSeenSQLInfoBox') === 'false'
+    ) {
+      setIsLanguageReferenceOpen(true);
+      window.localStorage.setItem('hasSeenSQLInfoBox', 'true');
+    } else if (
+      props.selectedLanguage === 'PPL' &&
+      window.localStorage.getItem('hasSeenPPLInfoBox') === 'false'
+    ) {
+      setIsLanguageReferenceOpen(true);
+      window.localStorage.setItem('hasSeenPPLInfoBox', 'true');
+    }
+  }, [props.selectedLanguage]);


Why do we need to specify this here? can we make it generic. Something like this?

Suggested change

useEffect(() => {

if (

props.selectedLanguage === 'SQL' &&

window.localStorage.getItem('hasSeenSQLInfoBox') === 'false'

) {

setIsLanguageReferenceOpen(true);

window.localStorage.setItem('hasSeenSQLInfoBox', 'true');

} else if (

props.selectedLanguage === 'PPL' &&

window.localStorage.getItem('hasSeenPPLInfoBox') === 'false'

) {

setIsLanguageReferenceOpen(true);

window.localStorage.setItem('hasSeenPPLInfoBox', 'true');

}

}, [props.selectedLanguage]);

useEffect(() => {

if (autoShow) {

const storageKey = `hasSeenInfoBox_${selectedLanguage}`;

const hasSeenInfoBox = window.localStorage.getItem(storageKey);

if (hasSeenInfoBox === 'false') {

setIsLanguageReferenceOpen(true);

window.localStorage.setItem(storageKey, 'true');

}

}

}, [props.selectedLanguage]);

Good catch. I just applied the change

ashwin-pc · 2024-10-27T01:18:13Z

src/plugins/query_enhancements/public/plugin.tsx

+    // Initialize local storage keys when the plugin loads, if they don't exist
+    if (!window.localStorage.getItem('hasSeenSQLInfoBox')) {
+      window.localStorage.setItem('hasSeenSQLInfoBox', 'false');
+    }
+    if (!window.localStorage.getItem('hasSeenPPLInfoBox')) {
+      window.localStorage.setItem('hasSeenPPLInfoBox', 'false');
+    }


Why do we ned this? why not just use undefined as the intiial state. If the key does not exist, its false by default

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc

I'm sorry i missed this in the first review, but we should not be reregistering the language every time the query updates.

ashwin-pc · 2024-10-27T22:41:11Z

src/plugins/data/public/query/query_string/language_service/lib/language_reference.test.tsx

+    cleanup();
+  });
+
+  test('auto-opens the info box on first load if autoShow is true and sets localStorage key', async () => {


Not a blocker, but can you add a fast follow test to ensure that this only opens once. Thats the key behavior here. Lets not forget to test that.

Sure lets add that case as a followup

ashwin-pc · 2024-10-27T22:42:28Z

src/plugins/data/public/query/query_string/language_service/lib/language_reference.tsx

Hmm, weird why its still complaining that these lines arent tested. Your tests have covered these lines

ashwin-pc · 2024-10-27T22:45:09Z

src/plugins/query_enhancements/public/plugin.tsx

-    const pplControls = [pplLanguageReference()];
-    const sqlControls = [sqlLanguageReference()];
+    // Subscribe to updates to track language changes
+    queryString.getUpdates$().subscribe((query) => {


Oh no, i missed this part. This is risky. We should not be reregistering the language every time the query changes! Did you sanity test discover with this change?

You can avoid this logic entirely since you only need query for the selected language, but looking at the code, you dont really need it right? on line 51&52, you can remove selectedLanguage since each instance of LanguageReference is created separately for that language.

fixed. I removed the subscriber

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc

Thanks! This looks much better 😊

ashwin-pc · 2024-10-27T23:43:54Z

src/plugins/query_enhancements/public/plugin.tsx

+    const pplControls = [pplLanguageReference('PPL')];
+    const sqlControls = [sqlLanguageReference('SQL')];


Nit: you dont even need this since pplLanguageReference will always only be used for PPL right? the additional prop you are sending is redundant

ashwin-pc · 2024-10-27T23:48:03Z

src/plugins/query_enhancements/public/plugin.tsx

-        filterable: false,
-        visualizable: false,
-      },
+      search: new PPLSearchInterceptor({


Nit: This is a nice change, but when making a change, keep the scope limited to just whats changing since its harder to review the diff quickly. Especially when there is no functinal benifit from the change. Thsi is just my personal opinion, so take it with a grain of salt.

ruanyl · 2024-10-28T03:21:07Z

src/plugins/data/public/query/query_string/language_service/lib/language_reference.test.tsx

+
+import React from 'react';
+import { render, screen, fireEvent, cleanup, act, waitFor } from '@testing-library/react';
+import '@testing-library/jest-dom';


Guess you don't need to import this? Because @testing-library/jest-dom is imported in jest setup files.

ruanyl · 2024-10-28T03:40:16Z

src/plugins/data/public/query/query_string/language_service/lib/language_reference.tsx

+      const storageKey = `hasSeenInfoBox_${props.selectedLanguage}`;
+      const hasSeenInfoBox = window.localStorage.getItem(storageKey);
+
+      if (hasSeenInfoBox === null && props.autoShow) {


It doesn't seem to need check props.autoShow here again because it's checked on line 21

Suggested change

if (hasSeenInfoBox === null && props.autoShow) {

if (hasSeenInfoBox === null) {

* [Discover] Modify the search bar info box content for sql/ppl Signed-off-by: Ryan Liang <[email protected]> * Remove comments Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 0 Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 1 Signed-off-by: Jialiang Liang <[email protected]> * Remove the un-used comment and update snapshots Signed-off-by: Jialiang Liang <[email protected]> * Rename SQL into OpenSearch SQL Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 0 Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 1 Signed-off-by: Jialiang Liang <[email protected]> * Enhancements + tests Signed-off-by: Jialiang Liang <[email protected]> * Changeset file for PR #8708 created/updated * remove comment Signed-off-by: Jialiang Liang <[email protected]> * Resolve comments Signed-off-by: Jialiang Liang <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Jialiang Liang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> (cherry picked from commit 90c40da) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…arch-project#8708) * [Discover] Modify the search bar info box content for sql/ppl Signed-off-by: Ryan Liang <[email protected]> * Remove comments Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 0 Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 1 Signed-off-by: Jialiang Liang <[email protected]> * Remove the un-used comment and update snapshots Signed-off-by: Jialiang Liang <[email protected]> * Rename SQL into OpenSearch SQL Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 0 Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 1 Signed-off-by: Jialiang Liang <[email protected]> * Enhancements + tests Signed-off-by: Jialiang Liang <[email protected]> * Changeset file for PR opensearch-project#8708 created/updated * remove comment Signed-off-by: Jialiang Liang <[email protected]> * Resolve comments Signed-off-by: Jialiang Liang <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Jialiang Liang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

…t for sql/ppl (#8762) * [Discover] Modify the search bar info box content for sql/ppl (#8708) * [Discover] Modify the search bar info box content for sql/ppl Signed-off-by: Ryan Liang <[email protected]> * Remove comments Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 0 Signed-off-by: Ryan Liang <[email protected]> * First time pop up - 1 Signed-off-by: Jialiang Liang <[email protected]> * Remove the un-used comment and update snapshots Signed-off-by: Jialiang Liang <[email protected]> * Rename SQL into OpenSearch SQL Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 0 Signed-off-by: Jialiang Liang <[email protected]> * Address comments - 1 Signed-off-by: Jialiang Liang <[email protected]> * Enhancements + tests Signed-off-by: Jialiang Liang <[email protected]> * Changeset file for PR #8708 created/updated * remove comment Signed-off-by: Jialiang Liang <[email protected]> * Resolve comments Signed-off-by: Jialiang Liang <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Jialiang Liang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com> * Add test id for languageReferenceButton Signed-off-by: Jialiang Liang <[email protected]> --------- Signed-off-by: Ryan Liang <[email protected]> Signed-off-by: Jialiang Liang <[email protected]> Co-authored-by: opensearch-changeset-bot[bot] <154024398+opensearch-changeset-bot[bot]@users.noreply.github.com>

Gokul-Radhakrishnan · 2024-12-03T18:42:51Z

opensearch-release-notes-2.18.0.md

How do we enable this experimental feature?

What is the name of this feature?

RyanL1997 · 2024-12-06T22:03:01Z

Hi @Gokul-Radhakrishnan , thanks for dropping the above comment. May I ask if the above question is related to the specific implementation of this PR? If it is only a general question that related to Discover feature for version 2.18.0, I suggest that to file an separate issue under OpenSearch Dashboards repo, so that more maintainers can be looped into the conversation to answer your questions.

And here are some useful links that related to the issue filing:

Where to open an issue: https://github.com/opensearch-project/OpenSearch-Dashboards/issues/new/choose
Documentation for how to contribute: https://github.com/opensearch-project/OpenSearch-Dashboards/blob/main/CONTRIBUTING.md#contributing-code

cc @ashwin-pc

Gokul-Radhakrishnan · 2024-12-16T13:27:23Z

@RyanL1997 I want to try out the PPL in Discover. How can I do that?

[Discover] Modify the search bar info box content for sql/ppl

91754eb

Signed-off-by: Ryan Liang <[email protected]>

github-actions bot added the valued-contributor label Oct 26, 2024

github-actions bot added the failed changeset label Oct 26, 2024

RyanL1997 added 3 commits October 26, 2024 00:28

Remove comments

3e7ce95

Signed-off-by: Ryan Liang <[email protected]>

First time pop up - 0

ccd480e

Signed-off-by: Ryan Liang <[email protected]>

First time pop up - 1

e70ee43

Signed-off-by: Jialiang Liang <[email protected]>

Remove the un-used comment and update snapshots

802ee69

Signed-off-by: Jialiang Liang <[email protected]>

RyanL1997 marked this pull request as ready for review October 27, 2024 00:11

RyanL1997 requested review from ananzh, kavilla, AMoo-Miki, ashwin-pc, joshuarrrr, abbyhu2000, zengyan-amazon, zhongnansu, manasvinibs, ZilongX, Flyingliuhub, curq, bandinib-amzn, SuZhou-Joe, ruanyl, BionIT, xinruiba, zhyuanqi and mengweieric as code owners October 27, 2024 00:11

RyanL1997 requested a review from huyaboo as a code owner October 27, 2024 00:11

Rename SQL into OpenSearch SQL

f332b91

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc reviewed Oct 27, 2024

View reviewed changes

RyanL1997 and others added 4 commits October 27, 2024 13:26

Address comments - 0

b41d451

Signed-off-by: Jialiang Liang <[email protected]>

Address comments - 1

802485b

Signed-off-by: Jialiang Liang <[email protected]>

Enhancements + tests

78cb904

Signed-off-by: Jialiang Liang <[email protected]>

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

55c6ea0

github-actions bot removed the failed changeset label Oct 27, 2024

remove comment

c7af324

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc requested changes Oct 27, 2024

View reviewed changes

Resolve comments

33b1ed2

Signed-off-by: Jialiang Liang <[email protected]>

ashwin-pc approved these changes Oct 27, 2024

View reviewed changes

ruanyl added backport 2.x v2.19.0 labels Oct 28, 2024

ruanyl approved these changes Oct 28, 2024

View reviewed changes

ruanyl merged commit 90c40da into opensearch-project:main Oct 28, 2024
72 checks passed

opensearch-trigger-bot bot mentioned this pull request Oct 28, 2024

[Backport 2.x] [Discover] Modify the search bar info box content for sql/ppl #8715

Closed

ananzh added v2.18.0 and removed v2.19.0 labels Oct 30, 2024

RyanL1997 mentioned this pull request Oct 30, 2024

[Manual Backport 2.x][Discover] Modify the search bar info box content for sql/ppl #8762

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Modify the search bar info box content for sql/ppl #8708

[Discover] Modify the search bar info box content for sql/ppl #8708

RyanL1997 commented Oct 26, 2024 •

edited

Loading

github-actions bot commented Oct 26, 2024

codecov bot commented Oct 26, 2024 •

edited

Loading

github-actions bot commented Oct 26, 2024

github-actions bot commented Oct 27, 2024

RyanL1997 commented Oct 27, 2024

github-actions bot commented Oct 27, 2024

ashwin-pc Oct 27, 2024

RyanL1997 Oct 27, 2024

ashwin-pc Oct 27, 2024

RyanL1997 Oct 27, 2024

ashwin-pc left a comment

ashwin-pc Oct 27, 2024

RyanL1997 Oct 27, 2024

ashwin-pc Oct 27, 2024

ashwin-pc Oct 27, 2024

ashwin-pc Oct 27, 2024

RyanL1997 Oct 27, 2024

ashwin-pc left a comment

ashwin-pc Oct 27, 2024

ashwin-pc Oct 27, 2024

ruanyl Oct 28, 2024

ruanyl Oct 28, 2024

Gokul-Radhakrishnan commented Dec 3, 2024

RyanL1997 commented Dec 6, 2024

Gokul-Radhakrishnan commented Dec 16, 2024

		const pplControls = [pplLanguageReference('PPL')];
		const sqlControls = [sqlLanguageReference('SQL')];

	if (hasSeenInfoBox === null && props.autoShow) {
	if (hasSeenInfoBox === null) {

[Discover] Modify the search bar info box content for sql/ppl #8708

[Discover] Modify the search bar info box content for sql/ppl #8708

Conversation

RyanL1997 commented Oct 26, 2024 • edited Loading

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Oct 26, 2024

❌ Empty Changelog Section

codecov bot commented Oct 26, 2024 • edited Loading

Codecov Report

github-actions bot commented Oct 26, 2024

❌ Empty Changelog Section

github-actions bot commented Oct 27, 2024

❌ Empty Changelog Section

RyanL1997 commented Oct 27, 2024

github-actions bot commented Oct 27, 2024

❌ Empty Changelog Section

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

ashwin-pc left a 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

ashwin-pc left a 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

Gokul-Radhakrishnan commented Dec 3, 2024

RyanL1997 commented Dec 6, 2024

Gokul-Radhakrishnan commented Dec 16, 2024

RyanL1997 commented Oct 26, 2024 •

edited

Loading

codecov bot commented Oct 26, 2024 •

edited

Loading