Kamu UI 423 query explainer UI #452
Conversation
| @@ -46,6 +47,11 @@ export const routes: Routes = [ | |||
| path: ProjectLinks.URL_ADMIN_DASHBOARD, | |||
| component: AdminDashboardComponent, | |||
| }, | |||
| { | |||
| canActivate: [AdminGuard], | |||
There was a problem hiding this comment.
The ticket is not saying anything that this new page should be limited to admins
There was a problem hiding this comment.
I will clarify the requirements
| <div | ||
| class="alert alert-info message-success mx-4 mt-4 fs-18 w-50 mx-auto d-flex align-items-center justify-content-center" | ||
| role="alert" | ||
| *ngIf="verifyResponse?.ok" |
There was a problem hiding this comment.
You've checked verifyResponse in the upper tag. If you are here, it cannot be undefined or null.
Correct everywhere.
| private sqlQuery: MaybeNull<string>; | ||
| public readonly DATE_FORMAT = AppValues.DISPLAY_FLOW_DATE_FORMAT; | ||
|
|
||
| public sqlQueryExplainer: QueryExplainerResponse; |
There was a problem hiding this comment.
Use word "response" in the variable name too. Right now it looks like it's a component
| public sqlQueryExplainer: QueryExplainerResponse; | ||
| public sqlQueryVerify$: Observable<VerifyQueryResponse>; | ||
| public verifyResponse: VerifyQueryResponse; |
There was a problem hiding this comment.
So input sqlQuery may be null, but the resulting data can't? I'm not sure this typing is accurate.
| .proccessQuery(this.sqlQuery) | ||
| .pipe( | ||
| tap((response) => { | ||
| this.sqlQueryExplainer = response; |
There was a problem hiding this comment.
Why can't we move the response into a separate observable, perhaps using sharedReplay operator to reuse the value?
| return of(e.error as VerifyQueryResponse); | ||
| }), | ||
| tap((data) => { | ||
| this.verifyResponse = data; |
There was a problem hiding this comment.
Again, this should be stored as an observable.
| tap((data) => { | ||
| this.verifyResponse = data; | ||
| }), | ||
| switchMap(() => forkJoin(blockHashObservables$)), |
There was a problem hiding this comment.
blockHashObservables$ as a local variable... this is whatever, but not functional reactive programming.
| const currentElement: HTMLButtonElement = event.currentTarget as HTMLButtonElement; | ||
| const currentElementChildren: HTMLCollectionOf<HTMLElement> = | ||
| currentElement.children as HTMLCollectionOf<HTMLElement>; | ||
| setTimeout(() => { | ||
| currentElementChildren[0].style.display = "inline-block"; | ||
| currentElementChildren[1].style.display = "none"; | ||
| currentElement.classList.remove("clipboard-btn--success"); | ||
| }, AppValues.LONG_DELAY_MS); | ||
| currentElementChildren[0].style.display = "none"; | ||
| currentElementChildren[1].style.display = "inline-block"; | ||
| currentElement.classList.add("clipboard-btn--success"); |
There was a problem hiding this comment.
Looks very low level and unclear.
Maybe naming the element's role and children roles may help to understand this.
| private extractSqlQueryFromRoute(): MaybeNull<string> { | ||
| const sqlQueryFromRoute: MaybeUndefined<string> = this.activatedRoute.snapshot.queryParams[ | ||
| ProjectLinks.URL_QUERY_PARAM_SQL_QUERY | ||
| ] as MaybeUndefined<string>; | ||
| return sqlQueryFromRoute ?? ""; | ||
| } |
There was a problem hiding this comment.
How it can be null, if you have a default value?
| } | ||
|
|
||
| public routeToDataset(alias: string): string { | ||
| return alias.includes("/") ? alias : `kamu/${alias}`; |
There was a problem hiding this comment.
Don't understand this. We have other users than kamu, what is the hack doing?
There was a problem hiding this comment.
If we use single-tenant image, we don't receive an account in the alias in the response.
Examlple:
account.tokens.portfolio - single-tenant image
kamu/account.tokens.portfolio - multi-tenant image
| public inputParamsHelper(option: string): string { | ||
| switch (option) { | ||
| case "SqlDataFusion": | ||
| return "Sql Data Fusion"; | ||
| case "JsonAoA": | ||
| return "Json AoA"; | ||
| case "ArrowJson": | ||
| return "Arrow Json"; | ||
| default: | ||
| return "Unknown options"; | ||
| } | ||
| } |
There was a problem hiding this comment.
A lot of doubt about spelling.
Sql => SQL
Data Fusion => DataFusion
Json => JSON
| private columnNames(): string[] { | ||
| return this.sqlQueryExplainer?.output?.schema.fields.map((item) => item.name) as string[]; | ||
| } |
There was a problem hiding this comment.
Make it accept a parameter that is already checked against null/undefined.
I.e. this.sqlQueryExplainer.output. could come from HTML template within checked divs.
| public get tableSource(): DataRow[] { | ||
| const result = this.sqlQueryExplainer?.output?.data.map((item) => { | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-return | ||
| return Object.assign({}, ...item.map((key, index) => ({ [this.columnNames()[index]]: key }))); | ||
| }) as DataRow[]; | ||
| return result; | ||
| } |
There was a problem hiding this comment.
When I see a type cast pragma, it means the code is wrong.
There was a problem hiding this comment.
Fixed, removed pragma
| dataFormat: "JsonAoA", | ||
| schemaFormat: "ArrowJson", |
There was a problem hiding this comment.
Don't we have these constants in GraphQL generated client?
There was a problem hiding this comment.
Yes there are, but only as enum keys
| queryDialect: string; | ||
| dataFormat: string; | ||
| schemaFormat: string; |
There was a problem hiding this comment.
These look like they are enums by nature
| type: string; | ||
| verificationMethod: string; |
There was a problem hiding this comment.
These look like enums too
There was a problem hiding this comment.
No, in this place string
| }; | ||
| output?: { | ||
| data: [string[]]; | ||
| dataFormat: string; |
There was a problem hiding this comment.
And this definitely is an enum
There was a problem hiding this comment.
Changed, set dataFormat: keyof typeof DataBatchFormat;
| error?: { | ||
| kind: string; | ||
| message: string; | ||
| actual_hash?: string; | ||
| expected_hash?: string; | ||
| dataset_id?: string; | ||
| block_hash?: string; | ||
| }; |
There was a problem hiding this comment.
As I understand, this is modeling a few types of errors which may nor may not have some of these parameters.
I prefer if we define a separate clean structure per each type of error that is hidden here, and union them in the field. The code that is using this will first test for kind, which is shared, and downcast to particular good error structures depending on the kind.
There was a problem hiding this comment.
Added separate error interface for all kind of errors
|
|
||
| public requestSystemTimeBlockByHash(datasetId: string, blockHash: string): Observable<string> { | ||
| return this.datasetApi.getSystemTimeBlockByHash(datasetId, blockHash).pipe( | ||
| map((data) => { | ||
| return data.datasets.byId?.metadata.chain.blockByHash?.systemTime ?? ""; | ||
| }), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Why time is returned as string? It should return time
There was a problem hiding this comment.
Fixed, changed to Date type
| </div> | ||
| </div> | ||
| </button> | ||
| </div> | ||
| <ng-container *ngIf="isAdmin"> |
There was a problem hiding this comment.
Again, where is the requirement written about opening this only for admins
There was a problem hiding this comment.
Right, only for admins
There was a problem hiding this comment.
-
I recommend to split the main observable into several reusable observables and avoiding state variables in the component. Use operators like
sharedReplaywhen reusing lower-level observables to avoid re-querying the source for multiple derived observables. -
The Query Explainer component looks very big to me. Consider splitting it into more manageable smaller parts.
Closes: #423
Result:
