Query requests return with sample size defined in advanced setting #9356

abbyhu2000 · 2025-02-07T23:30:39Z

Description

The size of the SQL and PPL responses should be determined by advanced setting discover:sampleSize. Default value is 500.

SQL:

Default

SELECT * FROM dataset

we use fetch_size param when sending SQL request to define how many hits we should get back:

PPL:

Default:

source = dataset | head ${advanced setting sampleSize value, default is 500}

fetch_size in sending PPL is currently broken due to [BUG] fetch_size does not work when sending PPL request sql#3314; until it get resolved, we interpolate the default PPL query to add a sample size for now
once the bug is resolved, we should change to use fetch_size just like how SQL does it. Created an issue to track this: [BUG] Remove limit size in default PPL query once fetch_size is working for PPL requests #9385

async SQL/PPL:

Default:

SELECT * FROM dataset LIMIT ${advanced setting sampleSize value, default is 500}
source = dataset | head ${advanced setting sampleSize value, default is 500}

fetch_size currently do not support for async queries; we interpolate the default queries to add a sample size

Screenshot

Screen.Recording.2025-02-13.at.4.56.28.PM.mov

Testing the changes

Changelog

feat: Remove sample size in default query string but use advanced setting

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

github-actions · 2025-02-07T23:31:13Z

❌ 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: abbyhu2000 <[email protected]>

codecov · 2025-02-07T23:46:52Z

Codecov Report

Attention: Patch coverage is 25.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 61.70%. Comparing base (0994dbe) to head (49a56fd).
Report is 10 commits behind head on main.

Files with missing lines	Patch %	Lines
...gins/query_enhancements/public/datasets/s3_type.ts	0.00%	3 Missing ⚠️
..._enhancements/server/search/sql_search_strategy.ts	33.33%	2 Missing ⚠️
...c/plugins/query_enhancements/server/utils/facet.ts	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9356      +/-   ##
==========================================
- Coverage   61.71%   61.70%   -0.01%     
==========================================
  Files        3816     3817       +1     
  Lines       91829    91847      +18     
  Branches    14543    14549       +6     
==========================================
+ Hits        56668    56670       +2     
- Misses      31506    31520      +14     
- Partials     3655     3657       +2

Flag	Coverage Δ
Linux_1	`28.98% <0.00%> (-0.01%)`	⬇️
Linux_2	`56.46% <ø> (ø)`
Linux_3	`39.19% <100.00%> (+0.01%)`	⬆️
Linux_4	`?`
Windows_1	`29.00% <0.00%> (-0.01%)`	⬇️
Windows_2	`56.41% <ø> (ø)`
Windows_3	`39.19% <100.00%> (+0.01%)`	⬆️
Windows_4	`28.89% <12.50%> (-0.02%)`	⬇️

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.

src/plugins/query_enhancements/public/datasets/s3_type.ts

Signed-off-by: abbyhu2000 <[email protected]>

…rch-Dashboards into sending_size_response

kavilla · 2025-02-10T19:50:53Z

src/plugins/query_enhancements/public/datasets/s3_type.ts

+  getInitialQueryString: (query: Query) => {
+    switch (query.language) {
+      case 'PPL':
+        return `source = ${query.dataset?.title} | head 10`;


be a follow up. but did want to potentially interpolate the sample size here?

so by default without changing the advance settings this would be source = foo | head 500?

@canascar however i dont know if that would cause some workload issues

kavilla · 2025-02-10T21:12:49Z

src/plugins/query_enhancements/server/utils/facet.ts

@@ -43,6 +43,7 @@ export class Facet {
      const { format, lang } = request.body;
      const params = {
        body: {
+          fetch_size: request.body.fetch_size,


if we have the context still here are we not able to just access advanced settings in this file and set the param? or was there a technical blocker that we should specify for each strategy the size if set or not.

and should we check if this value is set or not if it is then we pass it? what are implications of passing undefined to this the API? because right now i believe it will fire a request like

{ body: { fetch_size: undefined, query: query.query, // ... other properties }, // ... format property if applicable }

will it be ignored?

i dont see updates to the async search strategies (like S3), but I think doing it here in the fetch call should actually cover everything. Since async searches eventually hit this fetch method anyway, we should be good.

Mind double-checking that though? Just want to make sure I'm not missing something with the async flow

directly passing the fetch size in the facet will cause async query to fail since it doesn't accept fetch_size as a param

const fetchSize = await context.core.uiSettings.client.get('discover:sampleSize'); const params = { body: { fetch_size: fetchSize, query: query.query, ...(meta?.name && { datasource: meta.name }), ...(meta?.sessionId && { sessionId: meta.sessionId, }), ...(lang && { lang }), }, ...(format !== 'jdbc' && { format }), };

i think since async search for s3 and PPL doesn't support fetch_size yet, we can interpolate the default query string for s3 dataset types and PPL queries for now. The limit number we get from advanced setting

source = ${dataset.title} | head 500

Will change to only pass in fetch_size if fetch_size is defined.

kavilla · 2025-02-10T21:24:22Z

src/plugins/query_enhancements/server/search/ppl_search_strategy.ts

@@ -35,6 +35,7 @@ export const pplSearchStrategyProvider = (
  return {
    search: async (context, request: any, options) => {
      try {
+        request.body.fetch_size = await context.core.uiSettings.client.get('discover:sampleSize');


this is my bad - I get why you did it this way.

After thinking about it more, pulling from discover's advanced settings on the server side probably isn't the right move. If we want dashboards to have query editor + ppl/sql support down the road, we shouldn't make everything use discover's sample size setting.

however - until we deprecate these search strategies which is something we need to do and move to the right ones (based on data type + proper internal routes), we might be stuck. Could you check if we're already passing size params in the request/options somewhere? If we are, we should probably use that instead.

If we're not though, trying to refactor this now might be more trouble than it's worth, since we know we need to clean it up later anyway. Might make more sense to keep it on the query enhancements side for now and just add a TODO and link whatever issue we open in the code for follow up.

Let me know what you find re: the request params and we can figure out next steps.

Got it. By looking at the runSearch and fetch function in data plugin, i do not see we pass in any size param here.

Do you mind keep it on the query enhancements side for now?

kavilla · 2025-02-10T21:26:36Z

src/plugins/query_enhancements/server/search/sql_search_strategy.ts

@@ -33,6 +33,7 @@ export const sqlSearchStrategyProvider = (
  return {
    search: async (context, request: any, options) => {
      try {
+        request.body.fetch_size = await context.core.uiSettings.client.get('discover:sampleSize');


nit: instead of modifying objects directly, we should try to spread them like:
const newObj = { ...oldObj, newStuff }

It's a small thing but helps avoid unexpected side effects 👍

kavilla · 2025-02-10T21:27:52Z

...ion/core_opensearch_dashboards/opensearch_dashboards/apps/query_enhancements/queries.spec.js

@@ -98,7 +98,7 @@ const queriesTestSuite = () => {
        // Default SQL query should be set
        cy.waitForLoader(true);
        cy.getElementByTestId(`osdQueryEditor__multiLine`).contains(


js file 😭

kavilla · 2025-02-10T21:33:32Z

smart! some comments though first.

Signed-off-by: abbyhu2000 <[email protected]>

github-actions bot added the distinguished-contributor label Feb 7, 2025

abbyhu2000 added discover for discover reinvent backport 2.x labels Feb 7, 2025

github-actions bot added the failed changeset label Feb 7, 2025

remove sample size in query string but use advanced setting

b164a62

Signed-off-by: abbyhu2000 <[email protected]>

abbyhu2000 force-pushed the sending_size_response branch from d3fe9fb to b164a62 Compare February 7, 2025 23:32

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

f87789e

github-actions bot removed the failed changeset label Feb 7, 2025

abbyhu2000 commented Feb 7, 2025

View reviewed changes

src/plugins/query_enhancements/public/datasets/s3_type.ts Show resolved Hide resolved

abbyhu2000 added 2 commits February 8, 2025 00:24

add back limit and size 10 for s3 type

7976add

Signed-off-by: abbyhu2000 <[email protected]>

Merge branch 'sending_size_response' of github.com:abbyhu2000/OpenSea…

393d23f

…rch-Dashboards into sending_size_response

ananzh mentioned this pull request Feb 10, 2025

[Query Testing] TESTID-17: Test scenario #8934

Closed

kavilla reviewed Feb 10, 2025

View reviewed changes

abbyhu2000 added 2 commits February 14, 2025 00:22

address comments

d817b1d

Signed-off-by: abbyhu2000 <[email protected]>

minor typo

49a56fd

Signed-off-by: abbyhu2000 <[email protected]>

abbyhu2000 changed the title ~~Remove sample size in default query string but use advanced setting~~ Query requests return with sample size defined in advanced setting Feb 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query requests return with sample size defined in advanced setting #9356

Query requests return with sample size defined in advanced setting #9356

abbyhu2000 commented Feb 7, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025

codecov bot commented Feb 7, 2025 •

edited

Loading

kavilla Feb 10, 2025 •

edited

Loading

kavilla Feb 10, 2025

kavilla Feb 10, 2025 •

edited

Loading

abbyhu2000 Feb 13, 2025

abbyhu2000 Feb 13, 2025

abbyhu2000 Feb 13, 2025

kavilla Feb 10, 2025

abbyhu2000 Feb 13, 2025

kavilla Feb 10, 2025 •

edited

Loading

abbyhu2000 Feb 13, 2025

kavilla Feb 10, 2025

kavilla commented Feb 10, 2025

Query requests return with sample size defined in advanced setting #9356

Are you sure you want to change the base?

Query requests return with sample size defined in advanced setting #9356

Conversation

abbyhu2000 commented Feb 7, 2025 • edited Loading

Description

SQL:

PPL:

async SQL/PPL:

Screenshot

Testing the changes

Changelog

Check List

github-actions bot commented Feb 7, 2025

❌ Empty Changelog Section

codecov bot commented Feb 7, 2025 • edited Loading

Codecov Report

kavilla Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla Feb 10, 2025 • 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

kavilla Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kavilla commented Feb 10, 2025

abbyhu2000 commented Feb 7, 2025 •

edited

Loading

codecov bot commented Feb 7, 2025 •

edited

Loading

kavilla Feb 10, 2025 •

edited

Loading

kavilla Feb 10, 2025 •

edited

Loading

kavilla Feb 10, 2025 •

edited

Loading