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.

Sign up for GitHub

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

Jump to bottom

feat: add rows limit to query settings #1291

Merged
merged 21 commits into from
Sep 23, 2024
Merged

feat: add rows limit to query settings #1291

merged 21 commits into from
Sep 23, 2024

Conversation

astandrik
Copy link
Collaborator

@astandrik astandrik commented Sep 13, 2024 •
edited by github-actions bot
Loading

Closes #1253

Stand

CI Results

Test Status: ⚠️ FLAKY

📊 Full Report

Total Passed Failed Flaky Skipped
124 123 0 1 0

Bundle Size: 🔺

Current: 79.01 MB | Main: 78.99 MB
Diff: +0.02 MB (0.02%)

⚠️ Bundle size increased. Please review.

ℹ️ CI Information
  • Test recordings for failed tests are available in the full report.
  • Bundle size is measured for the entire 'dist' directory.
  • 📊 indicates links to detailed reports.
  • 🔺 indicates increase, 🔽 decrease, and ✅ no change in bundle size.

Raubzeug
Raubzeug requested changes Sep 13, 2024
View reviewed changes
src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx Outdated
name="limitRows"
control={control}
render={({field}) => (
<React.Fragment>
Copy link
Contributor

Choose a reason for hiding this comment

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

Fragment is redundant

src/containers/Tenant/Query/QuerySettingsDialog/QuerySettingsDialog.tsx
{QUERY_SETTINGS_FIELD_SETTINGS.limitRows.title}
</label>
<div className={b('control-wrapper')}>
<Controller
Copy link
Contributor

Choose a reason for hiding this comment

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

let's add validation in Controller?

src/store/reducers/executeQuery.ts Outdated
@@ -233,6 +233,7 @@ export const executeQueryApi = api.injectEndpoints({
querySettings.tracingLevel && enableTracingLevel
? TracingLevelNumber[querySettings.tracingLevel]
: undefined,
limit_rows: querySettings.limitRows || undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

will it be possible to run query without this setting at all?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

now it is running without this setting

or I didnt get your idea

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wanted to double-check that limit_rows may be undefined and nothing breaks!

@astandrik astandrik requested a review from ValeraS September 16, 2024 14:00
ValeraS
ValeraS requested changes Sep 16, 2024
View reviewed changes
Copy link
Collaborator

@ValeraS ValeraS left a comment

Choose a reason for hiding this comment

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

It seems you need to update some tests.

@astandrik
Copy link
Collaborator Author

@ValeraS Actually we cant use z.coerce as it changes the original values

@ValeraS
Copy link
Collaborator

ValeraS commented Sep 17, 2024

@ValeraS Actually we cant use z.coerce as it changes the original values

Why are timeout and rows limit not numbers?

@astandrik
Copy link
Collaborator Author

@ValeraS Actually we cant use z.coerce as it changes the original values

Why are timeout and rows limit not numbers?

I suppose it's because TextInput works with strings

@ValeraS
Copy link
Collaborator

ValeraS commented Sep 17, 2024

@ValeraS Actually we cant use z.coerce as it changes the original values

Why are timeout and rows limit not numbers?

I suppose it's because TextInput works with strings

How types of query options depend on view(TextInput)?

@astandrik
Copy link
Collaborator Author

@ValeraS Actually we cant use z.coerce as it changes the original values

Why are timeout and rows limit not numbers?

I suppose it's because TextInput works with strings

How types of query options depend on view(TextInput)?

no need in typecasts in view props

@astandrik astandrik requested a review from ValeraS September 18, 2024 13:25
ValeraS
ValeraS requested changes Sep 19, 2024
View reviewed changes
@astandrik astandrik requested a review from ValeraS September 19, 2024 10:30
@astandrik astandrik merged commit 1728e61 into main Sep 23, 2024
6 checks passed
@astandrik astandrik deleted the astandrik.add-rows-limit-to-query-settings-1253 branch September 23, 2024 13:26
@ydb-platform-bot ydb-platform-bot mentioned this pull request Sep 23, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValeraS ValeraS ValeraS approved these changes

@Raubzeug Raubzeug Raubzeug approved these changes

@artemmufazalov artemmufazalov Awaiting requested review from artemmufazalov

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add rows limit to query settings
3 participants
@astandrik @ValeraS @Raubzeug