feat(Queries): multiple full result [YTFRONT-5615]#1457
Open
SimbiozizV wants to merge 1 commit intomainfrom
Open
feat(Queries): multiple full result [YTFRONT-5615]#1457SimbiozizV wants to merge 1 commit intomainfrom
SimbiozizV wants to merge 1 commit intomainfrom
Conversation
Reviewer's GuideAdds support for multiple full-result tables in the query tracker UI, including existence checks in the backend flow and a new UI component to list, navigate to, and paste full-result table paths, plus engine feature metadata for capabilities. Sequence diagram for loading query result meta with multiple full results and existence checkssequenceDiagram
participant QueryResultsView
participant loadQueryResult
participant getQueryResultMeta
participant attachExistenceToFullResult
participant getClusterConfigByName
participant getClusterProxy
participant ytApiV3
QueryResultsView->>loadQueryResult: dispatch loadQueryResult(queryId, resultIndex)
activate loadQueryResult
loadQueryResult->>getQueryResultMeta: dispatch getQueryResultMeta(queryId, resultIndex)
activate getQueryResultMeta
getQueryResultMeta-->>loadQueryResult: meta
deactivate getQueryResultMeta
alt meta.full_result is defined
loadQueryResult->>attachExistenceToFullResult: attachExistenceToFullResult(meta.full_result)
activate attachExistenceToFullResult
attachExistenceToFullResult->>attachExistenceToFullResult: normalize to array items
loop for each item in items
attachExistenceToFullResult->>getClusterConfigByName: getClusterConfigByName(cluster)
getClusterConfigByName-->>attachExistenceToFullResult: clusterConfig
attachExistenceToFullResult->>getClusterProxy: getClusterProxy(clusterConfig)
getClusterProxy-->>attachExistenceToFullResult: proxy
attachExistenceToFullResult->>ytApiV3: ytApiV3.exists(proxy, path)
ytApiV3-->>attachExistenceToFullResult: boolean exists or error
attachExistenceToFullResult->>attachExistenceToFullResult: map to exist flag
end
attachExistenceToFullResult-->>loadQueryResult: full_result with exist flags
deactivate attachExistenceToFullResult
loadQueryResult->>loadQueryResult: meta.full_result = result
end
loadQueryResult-->>QueryResultsView: resultReady state with full_result and exist flags
deactivate loadQueryResult
QueryResultsView->>QueryReadyResultView: render with result and engine
QueryReadyResultView->>QueryFullResultList: render fullResult and engine
QueryFullResultList->>QueryFullResultRow: render rows for each full result item
Sequence diagram for user pasting a full result path into the query editorsequenceDiagram
actor User
participant QueryResultsView
participant QueryFullResultList
participant QueryFullResultRow
participant useMonaco
participant makePathByQueryEngine
participant insertTextWhereCursor
User->>QueryResultsView: Open query results page
QueryResultsView->>QueryFullResultList: Render with fullResult and engine
QueryFullResultList->>QueryFullResultRow: Render each row
User->>QueryFullResultRow: Click paste path button
QueryFullResultRow->>useMonaco: getEditor(queryEditor)
useMonaco-->>QueryFullResultRow: editor
QueryFullResultRow->>makePathByQueryEngine: makePathByQueryEngine(cluster, path, engine)
makePathByQueryEngine-->>QueryFullResultRow: engineSpecificPath
QueryFullResultRow->>insertTextWhereCursor: insertTextWhereCursor(engineSpecificPath, editor)
insertTextWhereCursor-->>QueryFullResultRow: text inserted
QueryFullResultRow-->>User: Path appears in query editor
Updated class diagram for query tracker full result metadata and UI componentsclassDiagram
class QueryFullResult {
string cluster
string table_path
boolean exist
}
class QueryResultMeta {
string id
number result_index
any schema
any yql_type
any statistics
any partition_statistics
QueryFullResult full_result
}
class QueryResultsView {
+render()
}
class QueryReadyResultView {
+props result
+props engine
+props onShowPreview
+render()
}
class QueryFullResultList {
+props fullResult
+props engine
+props className
+render()
}
class QueryFullResultRow {
+props id
+props engine
+props cluster
+props path
+props isExist
+handlePastePath()
+render()
}
class YqlEnginesInfo {
string[] available_yql_versions
string default_yql_ui_version
SupportedFeatures supported_features
}
class SupportedFeatures {
boolean declare_params
boolean multiple_full_results
boolean yql_runner
}
QueryResultMeta --> QueryFullResult : full_result 0..*
QueryResultsView --> QueryReadyResultView : renders
QueryReadyResultView --> QueryFullResultList : passes fullResult and engine
QueryFullResultList --> QueryFullResultRow : renders multiple
YqlEnginesInfo --> SupportedFeatures : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Storybook is ready. |
Contributor
|
Playwright components report is ready. |
Contributor
|
Statoscope report is ready. |
SimbiozizV
force-pushed
the
YTFRONT-5615
branch
from
745de2a to
af8e33d
Compare
Contributor
|
E2E-local report is ready. |
SimbiozizV
force-pushed
the
YTFRONT-5615
branch
from
af8e33d to
a475e2a
Compare
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
loadQueryResultyou mutatemeta.full_resultin place aftergetQueryResultMeta; consider cloning the meta object (or returning an enriched copy fromattachExistenceToFullResult) to avoid unexpected side effects if other code assumes query result metadata is immutable. - The user-facing text
Table has been removed from clusterinQueryFullResultRowis hard-coded; it would be better to hook this into the existing i18n system to keep translations consistent with the rest of the query tracker UI. - In
QueryFullResultList, thefullResultprop is typed asRequired<QueryResultMeta>['full_result']but you immediately normalize it to an array; you might simplify and make the component’s contract clearer by changing the prop type toQueryFullResult[]and doing the array normalization at the call site.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `loadQueryResult` you mutate `meta.full_result` in place after `getQueryResultMeta`; consider cloning the meta object (or returning an enriched copy from `attachExistenceToFullResult`) to avoid unexpected side effects if other code assumes query result metadata is immutable.
- The user-facing text `Table has been removed from cluster` in `QueryFullResultRow` is hard-coded; it would be better to hook this into the existing i18n system to keep translations consistent with the rest of the query tracker UI.
- In `QueryFullResultList`, the `fullResult` prop is typed as `Required<QueryResultMeta>['full_result']` but you immediately normalize it to an array; you might simplify and make the component’s contract clearer by changing the prop type to `QueryFullResult[]` and doing the array normalization at the call site.
## Individual Comments
### Comment 1
<location path="packages/ui/src/ui/pages/query-tracker/QueryResultsView/QueryFullResultList/QueryFullResultRow.tsx" line_range="40" />
<code_context>
+
+ return (
+ <div className={b()}>
+ <Text variant="subheader-1">Full Result {id}</Text>
+ {!isExist && <span>Table has been removed from cluster</span>}
+ {isExist ? (
</code_context>
<issue_to_address>
**suggestion:** Consider avoiding hardcoded English text to keep this view consistent with the rest of the localized UI.
This label is the only hardcoded English string in this view; the surrounding `QueryResults` content uses `i18n(...)`. Please wire this text through the same i18n mechanism (reusing an existing key if possible) for consistency with the localized UI.
Suggested implementation:
```typescript
return (
<div className={b()}>
<Text variant="subheader-1">
{i18n('query_tracker.full_result.title', 'Full Result')}{' '}
{id}
</Text>
{!isExist && (
<span>
{i18n(
'query_tracker.full_result.table-removed',
'Table has been removed from cluster',
)}
</span>
)}
```
1. Ensure `i18n` is imported in this file in the same way it is imported elsewhere in the Query Tracker UI (for example: `import {i18n} from '../../../../utils/i18n';` or the appropriate path/utility used in nearby components).
2. Add (or reuse) the keys `query_tracker.full_result.title` and `query_tracker.full_result.table-removed` in your localization resources, with translations for each supported language.
3. If your `i18n` helper uses a different signature (e.g. `i18n('key')` without a default string, or uses interpolation instead of concatenation), adjust the calls accordingly, potentially to something like `i18n('query_tracker.full_result.title', {id})`.
</issue_to_address>
### Comment 2
<location path="packages/ui/src/ui/pages/query-tracker/QueryResultsView/QueryFullResultList/QueryFullResultRow.tsx" line_range="41" />
<code_context>
+ return (
+ <div className={b()}>
+ <Text variant="subheader-1">Full Result {id}</Text>
+ {!isExist && <span>Table has been removed from cluster</span>}
+ {isExist ? (
+ <Link target="_blank" href={navigationLink}>
</code_context>
<issue_to_address>
**issue (review_instructions):** The user-facing message is slightly ungrammatical; it should read "removed from the cluster" rather than "removed from cluster".
To fix the grammar in this user-facing string, please add the article:
```tsx
{!isExist && <span>Table has been removed from the cluster</span>}
```
This keeps the meaning the same while correcting the English phrasing.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `packages/ui/**/*.js,packages/ui/**/*.ts,packages/ui/**/*.tsx,packages/ui/**/*.jsx`
**Instructions:**
Do not pass grammatical issues
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...es/ui/src/ui/pages/query-tracker/QueryResultsView/QueryFullResultList/QueryFullResultRow.tsx
Outdated
Show resolved
Hide resolved
...es/ui/src/ui/pages/query-tracker/QueryResultsView/QueryFullResultList/QueryFullResultRow.tsx
Outdated
Show resolved
Hide resolved
SimbiozizV
force-pushed
the
YTFRONT-5615
branch
from
a475e2a to
71b502e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://nda.ya.ru/t/mdLWDa4Q7WZwTD
## Summary by SourcerySupport displaying and handling multiple full query result tables, including existence checks and richer UI controls.
New Features:
Enhancements: