codeant-ai-for-open-source[bot] commented on code in PR #37516:
URL: https://github.com/apache/superset/pull/37516#discussion_r2747522761
##########
superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts:
##########
@@ -83,6 +83,23 @@ export interface ChartDataResponseResult {
* or null if multiple currencies are present.
*/
detected_currency?: string | null;
+ /**
+ * Query lifecycle timing breakdown in milliseconds.
+ */
+ timing?: {
+ /** Query object validation time */
+ validate_ms: number;
+ /** Cache lookup time */
+ cache_lookup_ms: number;
+ /** Database execution time (only present on cache miss) */
+ db_execution_ms?: number;
Review Comment:
**Suggestion:** `db_execution_ms` is optional (absent on cache hit) which
makes the JSON shape inconsistent and can cause client-side runtime errors when
consumers expect a numeric field; always include `db_execution_ms` but set it
to `null` on cache hits by typing it as `number | null` so clients can reliably
read the field. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Frontend timing math may produce NaN for cached results.
- ⚠️ UI diagnostics reading db_execution_ms can error.
- ⚠️ Type mismatch leads to unclear consumer contracts.
```
</details>
```suggestion
/** Database execution time (null when served from cache) */
db_execution_ms: number | null;
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger a cache-hit chart response from Superset backend (POST
/api/v1/chart/data) so
backend serves a cached result. The TS type for timing is declared in
`superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts:86-102`
where `db_execution_ms` is currently optional.
2. Inspect the JSON response for a cached result: the backend will omit
`timing.db_execution_ms` on cache hit per current implementation (matches
the optional
`db_execution_ms?: number` declaration in `QueryResponse.ts:86-102`).
3. A frontend consumer reading `queries[0].timing.db_execution_ms` without
guarding for
undefined will get `undefined`, which can produce NaN or runtime errors when
used in
arithmetic or display (code consuming this value would be reading the field
defined at
`QueryResponse.ts:86-102`).
4. Changing the type to `number | null` (and returning `null` on cache hits)
makes the
JSON shape consistent and forces explicit null checks in consumer logic,
avoiding
undefined-related runtime issues. The concrete type location is
`superset-frontend/.../QueryResponse.ts:86-102`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts
**Line:** 94:95
**Comment:**
*Possible Bug: `db_execution_ms` is optional (absent on cache hit)
which makes the JSON shape inconsistent and can cause client-side runtime
errors when consumers expect a numeric field; always include `db_execution_ms`
but set it to `null` on cache hits by typing it as `number | null` so clients
can reliably read the field.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
