-
Notifications
You must be signed in to change notification settings - Fork 16
Allow submitting statement w/o catalog/database #2404
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.
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modifies the Flink SQL statement submission workflow to allow users to submit statements without selecting a catalog and database, provided they use fully qualified table names. Previously, users were required to select both a compute pool and database before the "Submit Statement" action became available.
Key changes:
- Enable "Submit Statement" CodeLens action as soon as a compute pool is selected
- Add optional "Skip" functionality to database selection quickpick
- Handle optional database/catalog parameters in statement submission
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/quickpicks/kafkaClusters.ts |
Adds allowSkip option to kafka cluster quickpick with skip functionality |
src/quickpicks/kafkaClusters.test.ts |
Updates tests to verify skip option appears in quickpick items |
src/commands/flinkStatements.ts |
Makes database selection optional in statement submission flow |
src/codelens/flinkSqlProvider.ts |
Enables submit button when only compute pool is selected |
src/codelens/flinkSqlProvider.test.ts |
Adds test coverage for submit action with pool-only configuration |
| label: "Skip", | ||
| description: "Continue without selecting a database", | ||
| iconPath: new ThemeIcon("testing-skipped-icon"), | ||
| value: null as any, |
Copilot
AI
Aug 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using null as any bypasses TypeScript's type safety. Consider defining the type properly or using undefined consistently with the return type KafkaCluster | undefined."
| value: null as any, | |
| value: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derek1ee I disagree with Copilot here and would rather we use null explicitly so the extension can treat it as "user intentionally chose to skip" rather than the current cancellation treatment.
| * @param placeholder Optionally override the placeholder text for the quickpick. | ||
| * Defaults to "Select the Kafka cluster to use as the default database for the statement". | ||
| * @returns chosen Kafka cluster or undefined if the user cancels the quickpick. | ||
| * @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip". |
Copilot
AI
Aug 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc comment update is inconsistent with the actual return behavior. When "Skip" is selected, the function returns null (due to value: null as any), but the documentation says it returns undefined."
shouples
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @derek1ee! I like the idea of a dedicated skip option, but there are a few things that need to be updated for consistency and to make sure we're still following TypeScript best practices. I added a few suggestions below, and in addition I suggest:
- we update the return type of
kafkaClusterQuickPick()fromPromise<KafkaCluster | undefined>toPromise<KafkaCluster | null | undefined>:
vscode/src/quickpicks/kafkaClusters.ts
Lines 61 to 63 in 03eb5fa
export async function kafkaClusterQuickPick( options: KafkaClusterQuickPickOptions = {}, ): Promise<KafkaCluster | undefined> { - the command function for setting a document's database metadata should check
if (database !== undefined)instead of the more generalif (!database)(to allownull):
vscode/src/commands/documents.ts
Lines 101 to 104 in 03eb5fa
const database: KafkaCluster | undefined = await flinkDatabaseQuickpick(computePool); if (!database) { return; }
| ): Promise<KafkaCluster | undefined> { | ||
| return await kafkaClusterQuickPick({ | ||
| placeHolder: placeholder, | ||
| allowSkip: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also add allowSkip as a parameter to the flinkDatabaseQuickpick() function so it can be passed through from any callers. Currently, any callers of flinkDatabaseQuickpick() (like setting Flink defaults for the language server integration) will show the option to skip, and we only want this for the codelens implementation and setting document metadata, right?
| * @param placeholder Optionally override the placeholder text for the quickpick. | ||
| * Defaults to "Select the Kafka cluster to use as the default database for the statement". | ||
| * @returns chosen Kafka cluster or undefined if the user cancels the quickpick. | ||
| * @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @returns chosen Kafka cluster or undefined if the user cancels the quickpick or selects "Skip". | |
| * @returns chosen Kafka cluster, `null` if the user choose "Skip" (for Flink SQL statements), or undefined if the user cancels |
| export async function flinkDatabaseQuickpick( | ||
| computePool: CCloudFlinkComputePool, | ||
| placeholder: string = "Select the Kafka cluster to use as the default database for the statement", | ||
| ): Promise<KafkaCluster | undefined> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should update the return type of kafkaClusterQuickPick() as well as here:
| ): Promise<KafkaCluster | undefined> { | |
| ): Promise<KafkaCluster | null | undefined> { |
| label: "Skip", | ||
| description: "Continue without selecting a database", | ||
| iconPath: new ThemeIcon("testing-skipped-icon"), | ||
| value: null as any, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@derek1ee I disagree with Copilot here and would rather we use null explicitly so the extension can treat it as "user intentionally chose to skip" rather than the current cancellation treatment.
Summary of Changes
examplecatalog /marketplacedatabase.Any additional details or context that should be provided?
Submit action enabled as soon as a pool is selected:

Option to skip catalog/database selection after selecting submit statement:

Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Other
.vsixfile?