Run profile queries after fetching columns #2677
Conversation
| const ProfilingQueryExtractor = | ||
| /v1\/instances\/[a-zA-Z0-9-]+\/queries\/([a-zA-Z0-9-]+)\/tables\/(.+?)"/; | ||
| export function isProfilingQuery( | ||
| queryHash: string, | ||
| name: string, | ||
| ignoreProfileColumns = false | ||
| ) { | ||
| const queryExtractorMatch = ProfilingQueryExtractor.exec(queryHash); | ||
| if (!queryExtractorMatch) return false; | ||
| // TODO: move the query matching to a separate module and reuse in http queue | ||
| const [, type, table] = queryExtractorMatch; | ||
| return ( | ||
| table === name && (!ignoreProfileColumns || type !== "columns-profile") | ||
| ); |
There was a problem hiding this comment.
This is hard to follow. Could it be refactored to contain isTableProfileQuery and isColumnProfileQuery, or something along those lines?
| await queryClient.invalidateQueries({ | ||
| predicate: (query) => { | ||
| // TODO: move the query matching to a separate module and reuse in http queue | ||
| const queryExtractorMatch = ProfilingQueryExtractor.exec(query.queryHash); | ||
| if (!queryExtractorMatch) return false; | ||
| const [, type, table] = queryExtractorMatch; | ||
| return table === name && type === "columns-profile"; | ||
| }, | ||
| type: "active", | ||
| }); |
There was a problem hiding this comment.
Similar to isProfilingQuery, this is hard to follow. Could one function contain the logic, and you could call it here?
| @@ -113,6 +113,7 @@ | |||
| {hideRight} | |||
| {hideNullPercentage} | |||
| {compact} | |||
| enableProfiling={!$profileColumns?.isFetching} | |||
There was a problem hiding this comment.
Would it work with $profileColumns?.isSuccess or $profileColumns?.data? Either of those would be more semantically clear, IMO.
There was a problem hiding this comment.
enableProfiling has to be false when we are refetching profileColumns. isSuccess is true even while the profileColumns is being refetched.
|
I think this looks good. Besides my inline comments, a higher-level thought:
|
| const TopLevelProfilingQuery: Partial<Record<QueryRequestType, boolean>> = { | ||
| [QueryRequestType.TableColumns]: true, | ||
| [QueryRequestType.TableRows]: true, | ||
| }; |
There was a problem hiding this comment.
Does it make sense to include TableCardinality in this list? Then the variable could be TableProfilingQuery (not TopLevelProfilingQuery). It's not obvious to me why TableColumns and TableRows are in this unique group together.
* Run profile queries after fetching columns * Fixing prettier * Moving query matching to a new file * Fix lint * Updated E2E * Fix source profiling * TopLevelProfiling => TableProfiling
Checklist
Summary
Issue addressed:
closes #2662
Details:
Steps to Verify
select <column1> from <source>column1fromcolumn2by pasting and not typing. This way at a single shot the column is replaced.