Databricks sql warehouses #18273
Databricks sql warehouses #18273
Conversation
- Added create SQL warehouse action - Added list SQL warehouses action - Added get SQL warehouse action - Added edit SQL warehouse action - Added delete SQL warehouse action - Added start/stop SQL warehouse actions - Added get/set SQL warehouse config actions - Added get/set SQL warehouse permissions actions Implements 11 SQL Warehouses endpoints as discussed in the enhancement issue.
Summary This PR adds support for Databricks Vector Search Index APIs by implementing 9 new actions. Changes - Added create index action - Added get index action - Added list indexes action - Added delete index action - Added query index action - Added sync index action - Added scan index action - Added upsert index data action - Added delete index data action Resolves PipedreamHQ#18126
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
WalkthroughAdds a broad set of Databricks Vector Search APIs and actions (create/get/list/delete/query/sync/scan/upsert/delete-data) plus many SQL Warehouse actions (create/get/list/edit/delete/start/stop, config and permissions management), several action version bumps, and a package version bump to 0.4.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Action as Databricks Action
participant App as Databricks App
participant API as Databricks REST API
User->>Action: invoke action (indexName / warehouseId / params)
Action->>Action: parse & validate inputs (JSON arrays, vectors, keys, config)
Note right of Action #DDEBF7: compose payload (conditional fields: DELTA_SYNC vs DIRECT_ACCESS, config overrides)
Action->>App: call app method (create|get|list|query|sync|upsert|scan|delete|edit|set-config)
App->>API: HTTP request to Databricks endpoint
API-->>App: response
App-->>Action: return response
Action-->>User: return result + $.export("$summary", ...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Thank you so much for submitting this! We've added it to our backlog to review, and our team has been notified. |
|
Thanks for submitting this PR! When we review PRs, we follow the Pipedream component guidelines. If you're not familiar, here's a quick checklist:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/databricks/actions/list-runs/list-runs.mjs (2)
53-53: Bug: array compared to number in loop condition.
allRuns < this.maxResultsalways evaluates unexpectedly; should beallRuns.length.- } while (total === params.limit && allRuns < this.maxResults); + } while (total === params.limit && allRuns.length < this.maxResults);
1-66: Fix array-to-number comparison in loop condition ChangeallRuns < this.maxResultstoallRuns.length < this.maxResultsin components/databricks/actions/list-runs/list-runs.mjs around line 53.
🧹 Nitpick comments (32)
components/databricks/actions/run-job-now/run-job-now.mjs (1)
49-57: Guard against mutually exclusive params (jarParams vs notebookParams).The API rejects requests specifying both. Add a quick check to fail fast with a clear error.
async run({ $ }) { - const response = await this.databricks.runJobNow({ + const hasJar = Array.isArray(this.jarParams) && this.jarParams.length > 0; + const hasNotebook = this.notebookParams && Object.keys(this.notebookParams).length > 0; + if (hasJar && hasNotebook) { + throw new Error("jarParams and notebookParams are mutually exclusive. Provide only one."); + } + + const response = await this.databricks.runJobNow({ data: { job_id: this.jobId, jar_params: this.jarParams, notebook_params: this.notebookParams, python_params: this.pythonParams, spark_submit_params: this.sparkSubmitParams, idempotency_token: this.idempotencyToken, }, $, });components/databricks/actions/list-runs/list-runs.mjs (2)
33-38: Respect maxResults during pagination to avoid over-fetching.Adjust
limitto the remaining budget and stop early when the cap is reached.async run({ $ }) { - const params = { + const params = { job_id: this.jobId, active_only: this.activeOnly, - limit: 100, + limit: 100, offset: 0, }; + const max = this.maxResults ?? 100;
42-47: Cap each page by remaining results.Prevents extra API calls and trimming later.
- do { - const { runs } = await this.databricks.listRuns({ + do { + // Stop if we've already reached the cap + if (allRuns.length >= max) break; + // Limit this page to the remaining budget + params.limit = Math.min(params.limit, Math.max(1, max - allRuns.length)); + const { runs } = await this.databricks.listRuns({ params, $, });components/databricks/package.json (1)
3-3: Version bump — LGTM. Please ensure release notes.Given the sizable feature set, add/update CHANGELOG and fill in the PR description before merge.
components/databricks/common/constants.mjs (1)
1-11: Prevent accidental mutation of shared constants.Freeze the array to avoid runtime edits.
-export const CLUSTER_SIZES = [ +export const CLUSTER_SIZES = Object.freeze([ "2X-Small", "X-Small", "Small", "Medium", "Large", "X-Large", "2X-Large", "3X-Large", "4X-Large", -]; +]);components/databricks/common/utils.mjs (3)
16-24: Limit recursion to plain objects to avoid mangling Dates/Maps/Sets/etc.Current
typeof obj === "object"will traverse non-POJOs and strip their contents. Guard for plain objects.Apply:
- if (typeof obj === "object") { + if (typeof obj === "object" && Object.getPrototypeOf(obj) === Object.prototype) { return Object.fromEntries(Object.entries(obj).map(([ key, value, ]) => [ key, parseObject(value), ])); }
1-27: Protect against cyclic structures to prevent infinite recursion.Add a
WeakSetto track seen objects.One way:
-const parseObject = (obj) => { +const parseObject = (obj, _seen = new WeakSet()) => { @@ - if (Array.isArray(obj)) { - return obj.map((item) => parseObject(item)); + if (Array.isArray(obj)) { + return obj.map((item) => parseObject(item, _seen)); } @@ - if (typeof obj === "object" && Object.getPrototypeOf(obj) === Object.prototype) { + if (typeof obj === "object" && Object.getPrototypeOf(obj) === Object.prototype) { + if (_seen.has(obj)) return obj; + _seen.add(obj); return Object.fromEntries(Object.entries(obj).map(([ key, value, - ]) => [ - key, - parseObject(value), + ]) => [ + key, + parseObject(value, _seen), ])); }
29-31: Export named helper for ergonomics and tree-shaking.Apply:
-export default { - parseObject, -}; +export { parseObject }; +export default { parseObject };components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
16-17: Prefer nullish coalescing for count and keep concise pluralization.Apply:
- const count = response?.vector_indexes?.length || 0; - $.export("$summary", `Found ${count} vector search index${count === 1 ? "" : "es"}`); + const count = response?.vector_indexes?.length ?? 0; + $.export("$summary", `Found ${count} vector search index${count === 1 ? "" : "es"}`);components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
6-6: Clarify that deletion is irreversible in the description.Apply:
- description: "Deletes a vector search index in Databricks. [See the documentation](https://docs.databricks.com/api/workspace/vectorsearchindexes/deleteindex)", + description: "Irreversibly deletes a vector search index in Databricks. [See the documentation](https://docs.databricks.com/api/workspace/vectorsearchindexes/deleteindex)",components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
19-27: Optionally support waiting until the warehouse is ready.Starting is async; add a
waitForReadyflag to poll status viagetSQLWarehouseuntil RUNNING (with timeout).Apply:
props: { databricks, warehouseId: { description: "The ID of the SQL Warehouse to start", propDefinition: [ databricks, "warehouseId", ], }, + waitForReady: { + type: "boolean", + label: "Wait Until Ready", + description: "Polls the warehouse state until it's running (or times out).", + optional: true, + default: false, + }, + pollIntervalMs: { + type: "integer", + label: "Poll Interval (ms)", + optional: true, + default: 5000, + }, + timeoutMs: { + type: "integer", + label: "Timeout (ms)", + optional: true, + default: 300000, + }, }, async run({ $ }) { const response = await this.databricks.startSQLWarehouse({ warehouseId: this.warehouseId, $, }); $.export("$summary", `Successfully started SQL Warehouse with ID ${this.warehouseId}`); - return response; + if (!this.waitForReady) return response; + + const sleep = (ms) => new Promise((r) => setTimeout(r, ms)); + const start = Date.now(); + while (Date.now() - start < this.timeoutMs) { + const { state } = await this.databricks.getSQLWarehouse({ warehouseId: this.warehouseId, $ }); + if (state === "RUNNING") return { ...response, state }; + await sleep(this.pollIntervalMs); + } + throw new Error(`Timed out waiting for SQL Warehouse ${this.warehouseId} to be RUNNING`); },components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs (2)
9-18: Add an optional “idempotent delete” switchAllow users to treat 404 as success to make deletes safe to re-run.
Apply this diff to introduce an
ignoreMissingprop:props: { databricks, warehouseId: { description: "The ID of the SQL Warehouse to delete", propDefinition: [ databricks, "warehouseId", ], }, + ignoreMissing: { + type: "boolean", + label: "Ignore Missing", + description: "Treat 404 (not found) as success.", + optional: true, + }, },
19-29: Make deletion idempotent and improve error messagingCatch 404s when
ignoreMissingis enabled; keep other errors bubbling.Apply this diff:
- async run({ $ }) { - await this.databricks.deleteSQLWarehouse({ - warehouseId: this.warehouseId, - $, - }); - - $.export("$summary", `Successfully deleted SQL Warehouse with ID ${this.warehouseId}`); - return { - success: true, - }; - }, + async run({ $ }) { + try { + await this.databricks.deleteSQLWarehouse({ + warehouseId: this.warehouseId, + $, + }); + $.export("$summary", `Successfully deleted SQL Warehouse with ID ${this.warehouseId}`); + return { success: true }; + } catch (err) { + const status = err?.response?.status ?? err?.status; + if (this.ignoreMissing && status === 404) { + $.export("$summary", `Warehouse ${this.warehouseId} not found. Treated as success (ignored).`); + return { success: true, ignored: true }; + } + throw err; + } + },components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
131-152: Validate configuration pair items before sendingGuard against malformed entries to fail fast with actionable errors.
Apply this minimal validation:
- const configParam = utils.parseObject(this.configParam); + const configParam = utils.parseObject(this.configParam); if (Array.isArray(configParam) && configParam.length) { + configParam.forEach((p, i) => { + if (!p || typeof p !== "object" || typeof p.key !== "string") { + throw new ConfigurationError(`configParam[${i}] must be an object like { "key": string, "value": any }`); + } + }); payload.config_param = { configuration_pairs: configParam, }; } - const globalParam = utils.parseObject(this.globalParam); + const globalParam = utils.parseObject(this.globalParam); if (Array.isArray(globalParam) && globalParam.length) { + globalParam.forEach((p, i) => { + if (!p || typeof p !== "object" || typeof p.key !== "string") { + throw new ConfigurationError(`globalParam[${i}] must be an object like { "key": string, "value": any }`); + } + }); payload.global_param = { configuration_pairs: globalParam, }; } - const sqlConfigurationParameters = utils.parseObject(this.sqlConfigurationParameters); + const sqlConfigurationParameters = utils.parseObject(this.sqlConfigurationParameters); if (Array.isArray(sqlConfigurationParameters) && sqlConfigurationParameters.length) { + sqlConfigurationParameters.forEach((p, i) => { + if (!p || typeof p !== "object" || typeof p.key !== "string") { + throw new ConfigurationError(`sqlConfigurationParameters[${i}] must be an object like { "key": string, "value": any }`); + } + }); payload.sql_configuration_parameters = { configuration_pairs: sqlConfigurationParameters, }; } - const dataAccessConfig = utils.parseObject(this.dataAccessConfig); + const dataAccessConfig = utils.parseObject(this.dataAccessConfig); if (Array.isArray(dataAccessConfig) && dataAccessConfig.length) { payload.data_access_config = dataAccessConfig; }components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
19-27: Add user-friendly summaries for 404/403 (optional).Export clearer summaries on not found / forbidden without hiding the error.
- async run({ $ }) { - const response = await this.databricks.getSQLWarehousePermissions({ - warehouseId: this.warehouseId, - $, - }); - - $.export("$summary", `Retrieved permissions for SQL Warehouse ID ${this.warehouseId}`); - return response; - }, + async run({ $ }) { + try { + const response = await this.databricks.getSQLWarehousePermissions({ + warehouseId: this.warehouseId, + $, + }); + $.export("$summary", `Retrieved permissions for SQL Warehouse ID ${this.warehouseId}`); + return response; + } catch (err) { + if (err?.response?.status === 404) { + $.export("$summary", `SQL Warehouse ${this.warehouseId} not found.`); + } else if (err?.response?.status === 403) { + $.export("$summary", `Access denied to SQL Warehouse ${this.warehouseId}.`); + } + throw err; + } + },components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
25-26: Use resource name in summary when available (nit).Improves UX in runs list.
- $.export("$summary", `Retrieved details for SQL Warehouse ID ${this.warehouseId}`); + $.export("$summary", `Retrieved SQL Warehouse ${response?.name ?? this.warehouseId}`);components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (2)
9-18: Optional: add wait-for-stop toggle.Stopping is often async; offer a wait flag for convenience.
props: { databricks, warehouseId: { description: "The ID of the SQL Warehouse to stop", propDefinition: [ databricks, "warehouseId", ], }, + waitForStop: { + type: "boolean", + label: "Wait Until Stopped", + description: "Polls the warehouse status until STOPPED (or timeout).", + optional: true, + default: false, + }, + pollIntervalMs: { + type: "integer", + label: "Poll Interval (ms)", + optional: true, + default: 5000, + }, },
19-27: Optional: implement simple polling when wait is enabled.Poll status until STOPPED with a bounded loop.
async run({ $ }) { const response = await this.databricks.stopSQLWarehouse({ warehouseId: this.warehouseId, $, }); - $.export("$summary", `Successfully stopped SQL Warehouse with ID ${this.warehouseId}`); - return response; + if (!this.waitForStop) { + $.export("$summary", `Stop requested for SQL Warehouse ${this.warehouseId}`); + return response; + } + // Poll for terminal state + const maxTries = 60; // ~5 min with 5s default interval + for (let i = 0; i < maxTries; i++) { + const { state } = await this.databricks.getSQLWarehouse({ warehouseId: this.warehouseId, $ }); + if (state === "STOPPED") { + $.export("$summary", `SQL Warehouse ${this.warehouseId} is STOPPED.`); + return response; + } + await new Promise((r) => setTimeout(r, this.pollIntervalMs ?? 5000)); + } + $.export("$summary", `Timed out waiting for SQL Warehouse ${this.warehouseId} to stop.`); + return response; },components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (2)
39-41: Validate numResults > 0 (defensive).Prevent accidental zero/negative values.
- if (this.numResults !== undefined) { - body.num_results = this.numResults; - } + if (this.numResults !== undefined) { + if (!Number.isInteger(this.numResults) || this.numResults <= 0) { + throw new Error("numResults must be a positive integer."); + } + body.num_results = this.numResults; + }
35-47: Minor: rename body -> payload for consistency.Matches other actions’ naming.
- const body = {}; + const payload = {}; - if (typeof this.lastPrimaryKey === "string" && this.lastPrimaryKey.trim().length > 0) { - body.last_primary_key = this.lastPrimaryKey.trim(); + if (typeof this.lastPrimaryKey === "string" && this.lastPrimaryKey.trim().length > 0) { + payload.last_primary_key = this.lastPrimaryKey.trim(); } - if (this.numResults !== undefined) { - if (!Number.isInteger(this.numResults) || this.numResults <= 0) { - throw new Error("numResults must be a positive integer."); - } - body.num_results = this.numResults; + if (this.numResults !== undefined) { + if (!Number.isInteger(this.numResults) || this.numResults <= 0) { + throw new Error("numResults must be a positive integer."); + } + payload.num_results = this.numResults; } @@ - data: body, + data: payload,components/databricks/actions/sync-vector-search-index/sync-vector-search-index.mjs (1)
18-30: LGTM with a minor enhancement suggestionLooks good. Consider an optional boolean prop like
waitForCompletionto poll until the sync job reaches a terminal state and surface failures earlier, defaulting to fire-and-forget to preserve current behavior.components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (1)
42-53: Potential large-payload risk: consider chunking large upserts
inputs_jsoncan get big; APIs often have size limits. Consider batching (e.g., chunks of N rows or bytes) and invoking the endpoint per chunk, aggregating results.components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
64-70: Stricter validation for queryVector content (optional)After
JSON.parse, ensure it’s a non-empty array of numbers to catch silent shape issues early.if (this.queryVector) { try { payload.query_vector = JSON.parse(this.queryVector); + if (!Array.isArray(payload.query_vector) || !payload.query_vector.length || payload.query_vector.some((v) => typeof v !== "number")) { + throw new Error("`queryVector` must be a non-empty JSON array of numbers."); + } } catch (err) { throw new Error(`Invalid queryVector JSON: ${err.message}`); } }
86-88: Result count path may vary across API versionsIf responses use
resultsinstead ofresult.data_array, consider a fallback.- const count = response?.result?.data_array?.length || 0; + const count = response?.result?.data_array?.length + ?? response?.results?.length + ?? 0;components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
33-41: Optional: deduplicate ACL entries to avoid redundant updatesYou can collapse duplicates by principal + permission_level before sending.
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (4)
117-133: Validation is solid; consider aligning defaults and cross-field constraints
- You already enforce
maxNumClusters ≥ minNumClusters. Consider also failing fast whenmaxNumClustersis omitted butminNumClusters > 1and the backend requires both for autoscaling.- Optionally cap
num_clustersat backend-specific limits if they differ by edition.
149-158: Enforce PRO + serverless pairing (optional but user-friendly)When
enableServerlessCompute === true, validatewarehouseType === "PRO"to avoid backend errors.if (this.enableServerlessCompute !== undefined) payload.enable_serverless_compute = this.enableServerlessCompute; + if (this.enableServerlessCompute && this.warehouseType && this.warehouseType !== "PRO") { + throw new ConfigurationError("Serverless compute requires warehouseType = PRO."); + }
135-147: Tags parsing: guard undefined to avoid surprisesIf
this.tagsis undefined andutils.parseObjectexpects a string, default to{}.- const parsedTags = utils.parseObject(this.tags); + const parsedTags = utils.parseObject(this.tags || {});
156-158: Channel is already an object; parsing is optionalIf the prop type enforces an object,
utils.parseObjectis unnecessary. Harmless but can be dropped.components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (2)
140-146: Serverless + CLASSIC guard is good; consider auto-upgrading type (optional)Optional: if
enableServerlessCompute = trueandwarehouseTypenot provided, consider defaultingwarehouse_typetoPROto reduce user error, or add a message prompting users to set it.
148-160: Harden tags parsing and enforce tag count limit (< 45)
parseObjectonundefinedrelies on helper behavior; also enforce documented tag-count constraint and type-safety.Apply this diff:
- const parsedTags = utils.parseObject(this.tags); - const tagArray = Object.entries(parsedTags).map(([ - key, - value, - ]) => ({ - key, - value, - })); - if (tagArray.length) { - payload.tags = { - custom_tags: tagArray, - }; - } + const parsedTags = this.tags ? utils.parseObject(this.tags) : {}; + if (parsedTags && (typeof parsedTags !== "object" || Array.isArray(parsedTags))) { + throw new ConfigurationError("tags must be an object of key-value pairs."); + } + const tagEntries = Object.entries(parsedTags); + if (tagEntries.length > 45) { + throw new ConfigurationError("tags must contain fewer than 45 entries."); + } + const tagArray = tagEntries.map(([key, value]) => ({ key, value })); + if (tagArray.length) { + payload.tags = { custom_tags: tagArray }; + }components/databricks/databricks.app.mjs (1)
47-61: Nit: clarify prop descriptionThis looks copy-pasted from runs; it’s about Warehouses.
Apply this diff:
- description: "The ID of the SQL Warehouse to get runs from", + description: "The ID of the SQL Warehouse",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (27)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)components/databricks/actions/delete-sql-warehouse/delete-sql-warehouse.mjs(1 hunks)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs(1 hunks)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs(1 hunks)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-run-output/get-run-output.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs(1 hunks)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs(1 hunks)components/databricks/actions/list-runs/list-runs.mjs(1 hunks)components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs(1 hunks)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs(1 hunks)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs(1 hunks)components/databricks/actions/run-job-now/run-job-now.mjs(1 hunks)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs(1 hunks)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs(1 hunks)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs(1 hunks)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs(1 hunks)components/databricks/actions/sync-vector-search-index/sync-vector-search-index.mjs(1 hunks)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs(1 hunks)components/databricks/common/constants.mjs(1 hunks)components/databricks/common/utils.mjs(1 hunks)components/databricks/databricks.app.mjs(2 hunks)components/databricks/package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (18)
components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (3)
components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (1)
response(80-84)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (6)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (1)
response(109-112)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
response(14-14)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (1)
response(80-84)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (4)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(174-178)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(20-23)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)
components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (2)
components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
response(20-23)
components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (3)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(174-178)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
response(20-23)
components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)
components/databricks/common/utils.mjs (1)
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (2)
obj(110-110)obj(119-121)
components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (4)
components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (1)
response(80-84)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (5)
components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (1)
payload(42-45)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
response(14-14)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (7)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(80-84)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (1)
payload(42-45)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
response(14-14)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (4)
payload(105-108)parsedTags(135-135)tagArray(136-142)response(160-163)
components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (2)
payload(68-73)response(109-112)components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (4)
payload(106-106)parsedTags(148-148)tagArray(149-155)response(174-178)
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (3)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
response(13-15)components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
response(20-23)components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
response(33-39)
components/databricks/actions/sync-vector-search-index/sync-vector-search-index.mjs (5)
components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (1)
response(80-84)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
response(14-14)
components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (4)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (2)
payload(68-73)response(109-112)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(80-84)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-38)components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
response(19-22)
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (2)
components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (1)
response(174-178)components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs (1)
response(20-23)
🔇 Additional comments (25)
components/databricks/actions/run-job-now/run-job-now.mjs (1)
7-7: Version bump only — LGTM.components/databricks/actions/list-runs/list-runs.mjs (1)
55-57: Trimming fallback still fine — keep as a safety net.components/databricks/actions/get-run-output/get-run-output.mjs (1)
7-7: Version bump only — LGTM.components/databricks/common/constants.mjs (1)
13-15: Default export — LGTM.components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
13-20: LGTM.Straightforward call/summary/return flow is correct.
components/databricks/actions/delete-vector-search-index/delete-vector-search-index.mjs (1)
18-26: LGTM.Uses propDefinition, invokes app method, and exports a clear summary.
components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
19-30: LGTM.Consistent with other actions; summary is helpful.
components/databricks/actions/start-sql-warehouse/start-sql-warehouse.mjs (1)
7-7: Version bump sanity check.New action files elsewhere use 0.0.1; confirm 0.0.3 is intentional for this action.
components/databricks/actions/get-sql-warehouse-config/get-sql-warehouse-config.mjs (1)
12-18: LGTM — action wiring and summary are correctStraightforward call-through to the app method with a clear summary. Versioning and metadata look consistent with the suite.
components/databricks/actions/set-sql-warehouse-config/set-sql-warehouse-config.mjs (1)
77-88: Sanity-check allowed keys against Databricks SQL Warehouse API contract
No JSDoc or schema forsetSQLWarehouseConfigindatabricks.app.mjs; manually compare yourallowedlist with the official/sql/config/warehousesPUT request schema in the Databricks REST API docs or OpenAPI spec to ensure you’re not dropping any supported fields.components/databricks/actions/list-sql-warehouses/list-sql-warehouses.mjs (2)
12-16: LGTM on the listing call + shape.The call pattern and destructuring look good.
13-16: No action needed: listSQLWarehouses returns { warehouses } as expected
GET /2.0/sql/warehouses returns JSON of the form{ "warehouses": [ … ] }(learn.microsoft.com), andlistSQLWarehouses()simply returns that raw JSON.components/databricks/actions/get-sql-warehouse/get-sql-warehouse.mjs (1)
20-23: getSQLWarehouse signature validated
MethodgetSQLWarehouseincomponents/databricks/databricks.app.mjs:138destructures{ warehouseId, ...args }, so invoking it with{ warehouseId: this.warehouseId, $ }is correct.components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (1)
6-6: Confirm and align Stop API reference & states
- Update description link in components/databricks/actions/stop-sql-warehouse/stop-sql-warehouse.mjs (line 6) to the canonical Stop API docs: https://docs.databricks.com/sql/api/warehouses/stop
- Verify polling or summary logic uses
response.state === "STOPPED"as a terminal state alongside STARTING, RUNNING, and STOPPINGcomponents/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
49-53: No changes needed for response shape
The ScanVectorIndexResponse schema returns the entries inresponse.dataas a list, so the existing summary andresponse.data.lengthcheck are correct.components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (1)
26-41: Confirmedutils.parseObjecthandles arrays—no changes needed
parseObjectusesArray.isArrayto recursively map arrays, soparsedRowswill be an array whenrowsis an array or a JSON string thereof. Keep the existing array check.components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs (1)
26-33: No action required:utils.parseObjectalready performs recursive array mapping, so the subsequent filter covers empty or falsy entries correctly.components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (1)
110-115: Good guard on autoStopMinutesClear validation and mapping to
auto_stop_minslooks correct.components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (3)
75-81: Good: required field enforcement for DELTA_SYNCClear, early validation of
sourceTableforDELTA_SYNC.
114-118: Good: informative success summaryExports a concise, useful
$summary.
50-56: Fix prop type: embeddingSourceColumns should be object[] (not string[])The API expects an array of objects, but the prop is declared as
string[]and later treated as objects. This will yield a malformed payload at runtime.Apply this diff:
- embeddingSourceColumns: { - type: "string[]", + embeddingSourceColumns: { + type: "object[]", label: "Embedding Source Columns", description: - "List of embedding source column configs. Each entry should be a JSON object string like `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`", + "List of embedding source column configs. Example: `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`", optional: true, },⛔ Skipped due to learnings
Learnt from: GTFalcao PR: PipedreamHQ/pipedream#13961 File: components/fakturoid/actions/create-invoice/create-invoice.mjs:95-99 Timestamp: 2024-10-15T15:07:38.361Z Learning: An "object array" type is not yet supported for Pipedream props.components/databricks/actions/edit-sql-warehouse/edit-sql-warehouse.mjs (3)
108-121: Validation reads wellName length and
autoStopMinsconstraints are clear and enforceable.
123-138: Cluster bounds validation looks correctRange checks and
max >= minconstraint are accurate.
180-181: Good: succinct success summaryClear confirmation of the edited warehouse.
components/databricks/databricks.app.mjs (1)
62-75: LGTM: dynamic index optionsSimple, correct mapping of index names to option labels.
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
...nents/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/get-sql-warehouse-permissions/get-sql-warehouse-permissions.mjs
Show resolved
Hide resolved
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/set-sql-warehouse-permissions/set-sql-warehouse-permissions.mjs
Outdated
Show resolved
Hide resolved
...nents/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs
Show resolved
Hide resolved
...nents/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/databricks/actions/delete-endpoint/delete-endpoint.mjs (2)
4-6: Disambiguate action key/name to avoid confusion with Serving EndpointsThis action targets Vector Search Endpoints; the current key/name are generic and easy to confuse with ML Serving “endpoints.”
- key: "databricks-delete-endpoint", - name: "Delete Endpoint", + key: "databricks-delete-vector-search-endpoint", + name: "Delete Vector Search Endpoint",Note: renaming
keyis a breaking change for existing users; consider adding a new action instead if this is already published.
18-26: Consider idempotent delete + clearer summaryIf the API returns 404 for missing endpoints, allow users to treat that as success. Also, deletion may be async; “Requested deletion” is safer wording.
Apply within this block:
- async run({ $ }) { - const response = await this.databricks.deleteEndpoint({ - endpointName: this.endpointName, - $, - }); - - $.export("$summary", `Successfully deleted endpoint "${this.endpointName}".`); - return response; - }, + async run({ $ }) { + try { + const response = await this.databricks.deleteEndpoint({ + endpointName: this.endpointName, + $, + }); + $.export("$summary", `Requested deletion of endpoint "${this.endpointName}".`); + return response; + } catch (err) { + if (this.ignoreNotFound && err?.response?.status === 404) { + $.export("$summary", `Endpoint "${this.endpointName}" not found; treating as already deleted.`); + return { status: 404, message: "Not Found" }; + } + throw err; + } + },Add the optional prop (outside the selected range):
props: { databricks, endpointName: { propDefinition: [ databricks, "endpointName", ], }, + ignoreNotFound: { + type: "boolean", + label: "Ignore Not Found (404)", + description: "Treat a missing endpoint as success to make deletes idempotent.", + optional: true, + default: true, + }, },Please confirm whether the Databricks Delete Vector Search Endpoint API is async (returns 202/operation state). If synchronous, keep the original summary text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/databricks/actions/create-endpoint/create-endpoint.mjs(1 hunks)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs(1 hunks)components/databricks/actions/delete-endpoint/delete-endpoint.mjs(1 hunks)components/databricks/actions/get-endpoint/get-endpoint.mjs(1 hunks)components/databricks/actions/list-endpoints/list-endpoints.mjs(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- components/databricks/actions/list-endpoints/list-endpoints.mjs
- components/databricks/actions/get-endpoint/get-endpoint.mjs
- components/databricks/actions/create-endpoint/create-endpoint.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
🔇 Additional comments (2)
components/databricks/actions/delete-endpoint/delete-endpoint.mjs (2)
7-7: Version bump looks goodMetadata-only change. Aligned with a broader release bump.
6-6: Verify the doc URL pathDatabricks doc paths change occasionally; double-check this specific endpoint URL remains accurate and public.
|
Hii @jcortes could you please verify this PR . |
| type: "string[]", | ||
| label: "Primary Keys", | ||
| description: | ||
| "Values of the index’s primary key column to delete (e.g. `1`, `2`). These are the values for the column you set as `primary_key` when the index was created.", |
There was a problem hiding this comment.
| "Values of the index’s primary key column to delete (e.g. `1`, `2`). These are the values for the column you set as `primary_key` when the index was created.", | |
| "Values of the index's primary key column to delete (e.g. `1`, `2`). These are the values for the column you set as `primary_key` when the index was created.", |
jcortes
moved this from In Review
to Ready for QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from Ready for QA
to In QA
in Component (Source and Action) Backlog
vunguyenhung
moved this from In QA
to Changes Required
in Component (Source and Action) Backlog
|
Hello everyone, I have tested this PR and there're some test cases failed or needed improvement. Please check the test report below for more information |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (1)
56-61: Validate required inputs early: non-empty columns + at least one query modalityFail fast with clear messages before the API call.
async run({ $ }) { - const payload = { + if (!Array.isArray(this.columns) || this.columns.length === 0) { + throw new Error("`columns` must be a non-empty array of column names."); + } + if (!this.queryText && !this.queryVector) { + throw new Error("Provide at least one of `queryText` or `queryVector`."); + } + + const payload = { columns: this.columns, num_results: this.numResults, };
🧹 Nitpick comments (8)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
41-47: Constrain numResults to a sane lower boundPrevent 0 or negative requests at the prop level.
numResults: { type: "integer", label: "Number of Results", description: "Number of results to return. Defaults to 10.", optional: true, default: 10, + min: 1, },
29-34: Consider using an object prop for queryVector for better UXDeclaring this as
object(JSON) keeps UX consistent with filtersJson and avoids manual JSON typing by users.- queryVector: { - type: "string", + queryVector: { + type: "object", label: "Query Vector", - description: "JSON array of floats representing the embedding vector for the query.", + description: "Array of floats representing the embedding vector for the query.", optional: true, },If changed, also skip
JSON.parseand directly validatethis.queryVector.components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (5)
126-135: Validate schemaJson early; fail fast with clear errorPre-validate that schemaJson is valid JSON before sending to Databricks to avoid opaque 400s.
else if (this.indexType === "DIRECT_ACCESS") { if (!this.schemaJson) { throw new ConfigurationError( "schemaJson is required when indexType is DIRECT_ACCESS.", ); } + const schemaStr = this.schemaJson.trim(); + try { + JSON.parse(schemaStr); + } catch (e) { + throw new ConfigurationError(`schemaJson must be valid JSON: ${e.message}`); + } payload.direct_access_index_spec = { - schema_json: this.schemaJson, + schema_json: schemaStr, }; }
14-19: Align prop name with other actions: use indexName instead of nameOther vector index actions use indexName; keep UX consistent across actions.
- name: { + indexName: { type: "string", label: "Index Name", description: "A unique name for the index (e.g., `main_catalog.docs.en_wiki_index`).", },- const payload = { - name: this.name, + const payload = { + name: this.indexName, endpoint_name: this.endpointName, index_type: this.indexType, primary_key: this.primaryKey, };- `Successfully created vector search index: ${response?.name || this.name}`, + `Successfully created vector search index: ${response?.name || this.indexName}`,Also applies to: 81-87, 142-145
118-124: Ensure primaryKey is included in columnsToSync for DELTA_SYNCDatabricks requires the primary key be present in the synced columns; catch this locally.
payload.delta_sync_index_spec = { source_table: this.sourceTable, pipeline_type: this.pipelineType || "TRIGGERED", columns_to_sync: columnsToSync, embedding_source_columns: embeddingSourceColumns, }; + if (!columnsToSync.includes(this.primaryKey)) { + throw new ConfigurationError( + `columnsToSync must include the primaryKey "${this.primaryKey}".`, + ); + }
54-60: Clarify user-facing description for embeddingSourceColumnsRemove bracketed array example; each item is a single JSON object string. This reduces input mistakes.
embeddingSourceColumns: { type: "string[]", label: "Embedding Source Columns", description: - "List of embedding source column configs. Each entry should be a JSON object string like `[ { \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" } ]` (required for `DELTA_SYNC`).", + "List of embedding source column configs. Provide one JSON object per item, e.g. `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`. (Required for `DELTA_SYNC`.)", optional: true, },
89-116: Optional: validate embeddingSourceColumns object shapeConsider asserting required keys per item (e.g., embedding_model_endpoint_name and name/input column) to catch misconfigurations earlier.
If you want, I can add minimal shape checks once you confirm the exact field names per the Databricks API you’re targeting.
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
9-17: Optional: add a “limit” prop to cap results and shorten long pagination runs.Helpful for large workspaces and faster previews.
props: { databricks, endpointName: { propDefinition: [ databricks, "endpointName", ], }, + limit: { + type: "integer", + label: "Max Indexes to Return", + description: "Optional cap on the number of indexes returned. Leave empty to return all.", + optional: true, + default: 0, + min: 0, + }, }, async run({ $ }) { const allIndexes = []; let pageToken; do { const { vector_indexes, next_page_token, } = await this.databricks.listIndexes({ params: { endpoint_name: this.endpointName, ...(pageToken ? { page_token: pageToken, } : {}), }, $, }); if (vector_indexes?.length) { - allIndexes.push(...vector_indexes); + allIndexes.push(...vector_indexes); + if (this.limit && allIndexes.length >= this.limit) { + break; + } } pageToken = next_page_token; } while (pageToken);Also applies to: 19-43
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs(1 hunks)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (4)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(80-84)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (2)
payload(42-45)response(47-50)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-40)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Publish TypeScript components
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
🔇 Additional comments (5)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (4)
76-78: LGTM: includeEmbeddings passthrough is correctOnly emits the flag when set, preserving API defaults.
80-85: LGTM: API call wiringCorrect method, params, and $ passed through.
86-88: Use result.data_array — current Databricks "Query index" returns data_array
Databricks Vector Search returns a 2‑D result.data_array (rows align with manifest.columns and final element is similarity score); counting response?.result?.data_array?.length || 0 is correct — drop the matches/rows fallbacks.
7-7: Confirm doc link and correct filter fieldDatabricks: REST/CLI examples expect filters_json (JSON string); the Python SDK uses filters — ensure this action uses filters_json if it issues a REST/CLI JSON body, or filters if it calls the SDK. Location (for action text/link): components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (line 7).
description: "Query a specific vector search index in Databricks. [See the documentation](https://docs.databricks.com/api/workspace/vectorsearchindexes/queryindex)",components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
45-53: LGTM on summary and return shape.Concise summary and returning the aggregated array match Pipedream conventions.
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
|
Hey everyone , I’ve pushed the fixes. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (2)
26-27: Resolved previous mismatch with app method.Now correctly calling listVectorSearchIndexes; prior “listIndexes” mismatch is addressed.
6-6: Fix the docs URL (current link likely 404).Point to the stable CLI reference for vector search index listing and pagination.
Apply:
- description: "Lists all vector search indexes in the workspace. [See the documentation](https://docs.databricks.com/api/workspace/vectorsearchindexes/listindexes)", + description: "Lists all vector search indexes in the workspace. See the docs: https://docs.databricks.com/en/dev-tools/cli/reference/vector-search-indexes-commands",components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (1)
100-114: LGTM on hardened per-item JSON parsingThe try/catch per element with clear ConfigurationError messages addresses the earlier parsing failure mode well.
🧹 Nitpick comments (7)
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (2)
10-17: Add an optional maxPages safety guard.Prevents runaway loops if an API keeps returning the same page token.
Apply:
props: { databricks, endpointName: { propDefinition: [ databricks, "endpointName", ], }, + maxPages: { + type: "integer", + label: "Max Pages", + description: "Safety guard to prevent infinite pagination loops.", + optional: true, + default: 100, + }, },
20-22: Optional: enforce the maxPages guard in the loop.Implements the safety rail suggested above.
async run({ $ }) { const allIndexes = []; - let pageToken; + let pageToken; + let pages = 0; do { @@ - pageToken = next_page_token; + pageToken = next_page_token; + pages += 1; + if (pages >= this.maxPages) { + break; + } } while (pageToken);Also applies to: 42-43
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (5)
9-10: Update doc link to the current API referenceThe path in the description looks dated. Prefer referencing the REST Create Index endpoint explicitly to reduce confusion. For example: “POST /api/2.0/vector-search/indexes.” (docs.databricks.com)
54-60: Prefer object[] over string[] for JSON-shaped itemsEach item is a JSON object, but the prop type is string[]. Switching this to object[] avoids per-item JSON.parse and reduces user error. Keep backward compatibility by still accepting strings at runtime.
Apply this diff to the prop declaration:
- embeddingSourceColumns: { - type: "string[]", + embeddingSourceColumns: { + type: "object[]", label: "Embedding Source Columns", description: - "List of embedding source column configs. Each entry should be a JSON object string like `[ { \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" } ]` (required for `DELTA_SYNC`).", + "List of embedding source column configs. Each entry should be an object like `{ \"name\": \"text\", \"embedding_model_endpoint_name\": \"e5-small-v2\" }` (required for `DELTA_SYNC` when not using self-managed vectors).", optional: true, }, + // Optional: self-managed vectors (Delta Sync) + embeddingVectorColumns: { + type: "object[]", + label: "Embedding Vector Columns", + description: + "List of self-managed vector column configs. Each entry: `{ \"name\": \"text_vector\", \"embedding_dimension\": 1024 }` (required for `DELTA_SYNC` when not computing embeddings).", + optional: true, + },
134-143: Validate schemaJson early (optional) and keep sending it as a stringREST expects direct_access_index_spec.schema_json to be a JSON string. Consider parsing once to fail-fast on invalid JSON, then send the original string. (github.com)
Apply this diff:
- payload.direct_access_index_spec = { - schema_json: this.schemaJson, - }; + // Optional validation + try { + JSON.parse(this.schemaJson); + } catch (e) { + throw new ConfigurationError(`schemaJson is not valid JSON: ${e.message}`); + } + payload.direct_access_index_spec = { schema_json: this.schemaJson };
145-155: Add idempotency-friendly error message (optional)Consider catching “already exists” errors to return a clearer summary, since duplicate names are common during iteration. This is UX-only; no retries needed.
21-25: Surface endpoint type in endpointName options and enforce endpoint/indexType compatibilitypropDefinitions.endpointName currently returns only endpoint.name; STANDARD vs STORAGE_OPTIMIZED endpoints have different DELTA_SYNC semantics (columns_to_sync). Update the options to surface endpoint type in the label (e.g., "name (TYPE)") or filter options for DELTA_SYNC, and add runtime validation in create-vector-search-index to call getEndpoint and reject/inform on incompatible combinations or adjust help text for columnsToSync.
Locations: components/databricks/databricks.app.mjs (propDefinitions.endpointName) and components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (validate before create).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs(1 hunks)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (3)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(91-95)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (2)
payload(42-45)response(47-50)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (3)
1-1: LGTM: correct app import path.Relative path resolves to components/databricks/databricks.app.mjs from this action directory.
45-53: LGTM: clear summary and return shape.Summarizes total fetched and returns the aggregated array as expected.
24-36: No change required — fields and params match.listVectorSearchIndexes (components/databricks/databricks.app.mjs) forwards args to _makeRequest and is already consumed as { vector_indexes } / next_page_token; the action sends params.endpoint_name and params.page_token which are forwarded unchanged.
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (2)
47-60: Align prop descriptions with behaviorDescriptions still claim these are “required” for DELTA_SYNC, but code makes them optional and supports two modes. Clarify to reduce user confusion.
Apply this diff:
@@ columnsToSync: { type: "string[]", label: "Columns to Sync", description: - "List of columns to sync from the source Delta table. Example: `[\"id\", \"text\"]` (required for `DELTA_SYNC`).", + "Optional list of columns to sync from the source Delta table. Example: `[\"id\", \"text\"]` (DELTA_SYNC).", optional: true, }, embeddingSourceColumns: { type: "string[]", label: "Embedding Source Columns", description: - "List of embedding source column configs. Each entry should be a JSON object string like `[ { \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" } ]` (required for `DELTA_SYNC`).", + "List of embedding source column configs. Provide one JSON object string per element, e.g. `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`. Use this when Databricks computes embeddings (DELTA_SYNC).", optional: true, },
178-187: Validate schemaJson early to avoid opaque API failuresParse once to ensure it’s valid JSON, but still send the original string.
@@ - payload.direct_access_index_spec = { + // Validate schemaJson early; keep payload as string + try { + JSON.parse(this.schemaJson); + } catch (e) { + throw new ConfigurationError(`schemaJson must be valid JSON: ${e.message}`); + } + payload.direct_access_index_spec = { schema_json: this.schemaJson, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (4)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(91-95)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (2)
payload(42-45)response(47-50)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(105-108)response(160-163)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-40)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
🔇 Additional comments (2)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (2)
100-115: Good: hardened parsing with clear errorsPer-item JSON.parse with try/catch and indexed error messages is solid.
145-147: Good: include columns_to_sync only when providedThis matches flexible API behavior and avoids sending empty arrays.
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (5)
47-53: Description says “required” but the code treats columnsToSync as optionalThe runtime only sets columns_to_sync if provided. Align the prop description to avoid confusion.
- description: - "List of columns to sync from the source Delta table. Example: `[\"id\", \"text\"]` (required for `DELTA_SYNC`).", + description: + "Columns to sync from the source Delta table. Example: `[\"id\", \"text\"]`. Optional for `DELTA_SYNC`; include only when you want to restrict synced columns.",
68-74: Clarify that Schema JSON must be valid JSONSmall copy tweak to make expectations explicit.
- description: - "The schema of the index in JSON format. Example: `{ \"columns\": [{ \"name\": \"id\", \"type\": \"string\" }, { \"name\": \"text_vector\", \"type\": \"array<double>\" }] }`. Required for `DIRECT_ACCESS` indexes.", + description: + "The schema of the index in JSON format (must be valid JSON). Example: `{ \"columns\": [{ \"name\": \"id\", \"type\": \"string\" }, { \"name\": \"text_vector\", \"type\": \"array<double>\" }] }`. Required for `DIRECT_ACCESS` indexes.",
54-67: Consider letting users provide object[] directly for embedding config propsUsing type "object[]" would avoid forcing JSON strings and reduce parsing/UX friction. Keep current string[] parsing for backward compatibility if you switch types later.
157-159: Optionally auto-include primaryKey in columns_to_sync and validate column referencesIf columns_to_sync is provided but omits the primary key or referenced columns (from embedding configs) aren’t present, Databricks may reject or behave unexpectedly. Auto-including the PK is a safe default; warning on missing referenced columns helps users.
@@ - if (Array.isArray(columnsToSync) && columnsToSync.length > 0) { - deltaSpec.columns_to_sync = columnsToSync; - } + if (Array.isArray(columnsToSync) && columnsToSync.length > 0) { + const toSync = [ ...columnsToSync ]; + if (this.primaryKey && !toSync.includes(this.primaryKey)) { + toSync.unshift(this.primaryKey); + } + // Optional sanity check: ensure embedding column names are included + const referenced = [ + ...(hasSource ? embeddingSourceColumns.map((c) => c?.name).filter(Boolean) : []), + ...(hasVectors ? embeddingVectorColumns.map((c) => c?.name).filter(Boolean) : []), + ]; + for (const col of referenced) { + if (col && !toSync.includes(col)) { + // Not throwing to keep behavior lenient; switch to ConfigurationError if you want strictness. + $.export("$summary", `Note: columns_to_sync does not include referenced column "${col}". Consider adding it.`); + } + } + deltaSpec.columns_to_sync = toSync; + }Also applies to: 160-186
206-209: Minor: make summary more robust to varied response shapesFallback to nested name if the API wraps the index in an object.
- `Successfully created vector search index: ${response?.name || this.name}`, + `Successfully created vector search index: ${response?.name || response?.index?.name || this.name}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (5)
components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs (2)
payload(57-60)response(91-95)components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (2)
payload(42-45)response(47-50)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(105-108)response(160-163)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-40)components/databricks/actions/get-vector-search-index/get-vector-search-index.mjs (1)
response(20-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Publish TypeScript components
- GitHub Check: Verify TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Lint Code Base
🔇 Additional comments (3)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (3)
139-151: Good: enforce exactly one of source vs vector embedding configsMutual exclusivity and “at least one” checks look correct.
201-204: Verified — createVectorSearchIndex exists and action key is unique.createVectorSearchIndex is implemented in components/databricks/databricks.app.mjs (≈line 266). The action key "databricks-create-vector-search-index" appears only in components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (line 6).
9-10: Update Databricks create-index docs link & confirm payload fields
- Correct REST endpoint: POST /api/2.0/vector-search/indexes.
- Delta Sync request fields: source_table, primary_key, pipeline_type (CONTINUOUS|TRIGGERED), columns_to_sync (array), and either embedding_source_column (+ embedding_model_endpoint_name) OR embedding_vector_column (self-managed vectors).
- Direct Vector Access fields: index_name, primary_key, embedding_dimension, embedding_vector_column, and schema (object) or schema_json (JSON).
- Action — components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (lines 9–10): update the docs URL to the official create-index docs and ensure the payload uses embedding_source_column OR embedding_vector_column (not both).
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Show resolved
Hide resolved
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Outdated
Show resolved
Hide resolved
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs
Show resolved
Hide resolved
Lokeshchand33
force-pushed
the
databricks-sql-warehouses
branch
from
65629eb to
9ea188a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (4)
139-151: Mutual exclusivity check: good enforcement.Exactly-one-of validation matches Databricks behavior and prevents ambiguous configs.
103-106: Guard parseObject to avoid crashes when columnsToSync is omitted.columnsToSync is optional; calling utils.parseObject on undefined/empty can throw.
- const columnsToSync = Array.isArray(this.columnsToSync) - ? this.columnsToSync - : utils.parseObject(this.columnsToSync); + const columnsToSync = Array.isArray(this.columnsToSync) + ? this.columnsToSync + : (typeof this.columnsToSync === "string" && this.columnsToSync.trim() + ? utils.parseObject(this.columnsToSync) + : undefined);
107-137: Normalize and flatten array props; robust JSON parsing for embedding columns.Users often paste a whole JSON array as a single item; current logic can yield nested arrays or confusing errors. Normalize both props, flatten one level, and keep clear errors.
- // Parse embeddingSourceColumns (either object[] or string[] of JSON) - const embeddingSourceColumns = Array.isArray(this.embeddingSourceColumns) - ? this.embeddingSourceColumns.map((item, idx) => { - if (typeof item === "string") { - try { - return JSON.parse(item); - } catch (e) { - throw new ConfigurationError( - `embeddingSourceColumns[${idx}] is not valid JSON: ${e.message}`, - ); - } - } - return item; - }) - : utils.parseObject(this.embeddingSourceColumns); - - // Parse embeddingVectorColumns (either object[] or string[] of JSON) - const embeddingVectorColumns = Array.isArray(this.embeddingVectorColumns) - ? this.embeddingVectorColumns.map((item, idx) => { - if (typeof item === "string") { - try { - return JSON.parse(item); - } catch (e) { - throw new ConfigurationError( - `embeddingVectorColumns[${idx}] is not valid JSON: ${e.message}`, - ); - } - } - return item; - }) - : utils.parseObject(this.embeddingVectorColumns); + // Normalize a prop that may be: object[], string[] (JSON), or a single JSON string + const normalizeArrayProp = (prop, label) => { + if (Array.isArray(prop)) { + return prop.flatMap((item, idx) => { + if (typeof item === "string") { + try { + const parsed = JSON.parse(item); + return Array.isArray(parsed) ? parsed : [parsed]; + } catch (e) { + throw new ConfigurationError(`${label}[${idx}] is not valid JSON: ${e.message}`); + } + } + return [item]; + }); + } + if (typeof prop === "string" && prop.trim()) { + try { + const parsed = JSON.parse(prop); + return Array.isArray(parsed) ? parsed : [parsed]; + } catch (e) { + throw new ConfigurationError(`${label} is not valid JSON array: ${e.message}`); + } + } + return undefined; + }; + + const embeddingSourceColumns = normalizeArrayProp(this.embeddingSourceColumns, "embeddingSourceColumns"); + const embeddingVectorColumns = normalizeArrayProp(this.embeddingVectorColumns, "embeddingVectorColumns");
190-199: Validate schemaJson before API call to fail fast on bad JSON.Avoids opaque 4xx from the API and improves UX.
- payload.direct_access_index_spec = { - schema_json: this.schemaJson, - }; + // Validate schemaJson is valid JSON but preserve string payload shape for the API + try { + JSON.parse(this.schemaJson); + } catch (e) { + throw new ConfigurationError(`schemaJson must be valid JSON: ${e.message}`); + } + payload.direct_access_index_spec = { schema_json: this.schemaJson };
🧹 Nitpick comments (2)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (2)
47-53: Fix doc inconsistency: columnsToSync is optional in code but marked “required”.Runtime allows omitting columns_to_sync; update the prop description to match.
- description: - "List of columns to sync from the source Delta table. Example: `[\"id\", \"text\"]` (required for `DELTA_SYNC`).", + description: + "List of columns to sync from the source Delta table. Example: `[\"id\", \"text\"]`. Optional for `DELTA_SYNC`.",
54-60: Make mutual exclusivity explicit in prop docs (source vs vector columns).The runtime enforces exactly one; reflect that in descriptions to reduce user error.
- description: - "List of embedding source column configs. Each entry is a JSON object string like `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`. Provide when Databricks computes embeddings (DELTA_SYNC).", + description: + "List of embedding source column configs. Each entry is a JSON object string like `{ \"embedding_model_endpoint_name\": \"e5-small-v2\", \"name\": \"text\" }`. Use when Databricks computes embeddings (DELTA_SYNC). Mutually exclusive with “Embedding Vector Columns”.", @@ - description: - "List of self-managed vector column configs. Each entry is a JSON object string like `{ \"name\": \"text_vector\", \"embedding_dimension\": 1536 }`. Provide when you manage embeddings yourself (DELTA_SYNC).", + description: + "List of self-managed vector column configs. Each entry is a JSON object string like `{ \"name\": \"text_vector\", \"embedding_dimension\": 1536 }`. Use when you manage embeddings yourself (DELTA_SYNC). Mutually exclusive with “Embedding Source Columns”.",Also applies to: 61-67
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs(1 hunks)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs(1 hunks)components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/databricks/actions/query-vector-search-index/query-vector-search-index.mjs
- components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs
🧰 Additional context used
🧬 Code graph analysis (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (4)
components/databricks/actions/upsert-vector-search-index-data/upsert-vector-search-index-data.mjs (2)
payload(42-45)response(47-50)components/databricks/actions/create-sql-warehouse/create-sql-warehouse.mjs (2)
payload(105-108)response(160-163)components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs (1)
response(34-40)components/databricks/actions/scan-vector-search-index/scan-vector-search-index.mjs (1)
response(43-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
- GitHub Check: Verify TypeScript components
🔇 Additional comments (1)
components/databricks/actions/create-vector-search-index/create-vector-search-index.mjs (1)
160-186: Entry-level validations look solid.Required keys for both config modes are enforced with clear errors.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
components/databricks/databricks.app.mjs (4)
274-282: URL-encode dynamic indexName segmentsIndex names may contain characters that break paths. Encode them.
- path: `/vector-search/indexes/${indexName}`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}`, @@ - path: `/vector-search/indexes/${indexName}`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}`, @@ - path: `/vector-search/indexes/${indexName}/query`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/query`, @@ - path: `/vector-search/indexes/${indexName}/sync`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/sync`, @@ - path: `/vector-search/indexes/${indexName}/delete-data?primary_keys=${params.primary_keys}`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/delete-data?primary_keys=${encodeURIComponent(params?.primary_keys ?? "")}`, @@ - path: `/vector-search/indexes/${indexName}/upsert-data`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/upsert-data`, @@ - path: `/vector-search/indexes/${indexName}/scan`, + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/scan`,Also applies to: 294-302, 304-312, 314-322, 324-333, 335-343, 345-353
156-176: URL-encode dynamic endpointName segmentsSame safety concern applies to endpoint names.
- path: `/vector-search/endpoints/${endpointName}`, + path: `/vector-search/endpoints/${encodeURIComponent(endpointName)}`, @@ - path: `/vector-search/endpoints/${endpointName}`, + path: `/vector-search/endpoints/${encodeURIComponent(endpointName)}`,
179-187: URL-encode dynamic warehouseId across all SQL Warehouse pathsPrevent path issues if IDs contain reserved characters.
- path: `/sql/warehouses/${warehouseId}`, + path: `/sql/warehouses/${encodeURIComponent(warehouseId)}`, @@ - path: `/sql/warehouses/${warehouseId}`, + path: `/sql/warehouses/${encodeURIComponent(warehouseId)}`, @@ - path: `/sql/warehouses/${warehouseId}/edit`, + path: `/sql/warehouses/${encodeURIComponent(warehouseId)}/edit`, @@ - path: `/sql/warehouses/${warehouseId}/start`, + path: `/sql/warehouses/${encodeURIComponent(warehouseId)}/start`, @@ - path: `/sql/warehouses/${warehouseId}/stop`, + path: `/sql/warehouses/${encodeURIComponent(warehouseId)}/stop`, @@ - path: `/permissions/warehouses/${warehouseId}`, + path: `/permissions/warehouses/${encodeURIComponent(warehouseId)}`, @@ - path: `/permissions/warehouses/${warehouseId}`, + path: `/permissions/warehouses/${encodeURIComponent(warehouseId)}`,Also applies to: 188-196, 203-211, 212-220, 222-230, 247-265
68-72: Fix misleading Warehouse ID descriptionWarehouses don’t have “runs”. Simplify the text.
- description: "The ID of the SQL Warehouse to get runs from", + description: "The SQL Warehouse ID",components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
6-6: Fix docs URL (current link likely 404); point to stable referenceUse the CLI reference that documents list-indexes and pagination.
- description: "Lists all vector search indexes for a given endpoint. [See the documentation](https://docs.databricks.com/api/workspace/vectorsearchindexes/listindexes)", + description: "Lists all vector search indexes for a given endpoint. See: https://docs.databricks.com/dev-tools/cli/reference/vector-search-indexes-commands",
🧹 Nitpick comments (3)
components/databricks/databricks.app.mjs (2)
324-333: Build query via params and handle arrays; avoid “undefined” in URLSafer to pass query via
params, support array input, and avoid appendingundefined.- deleteVectorSearchData({ - indexName, params, ...args - }) - { - return this._makeRequest({ - path: `/vector-search/indexes/${indexName}/delete-data?primary_keys=${params.primary_keys}`, - method: "DELETE", - ...args, - }); - }, + deleteVectorSearchData({ indexName, params = {}, ...args }) { + const qp = {}; + if (params.primary_keys != null) { + qp.primary_keys = Array.isArray(params.primary_keys) + ? params.primary_keys.join(",") + : params.primary_keys; + } + return this._makeRequest({ + path: `/vector-search/indexes/${encodeURIComponent(indexName)}/delete-data`, + method: "DELETE", + params: qp, + ...args, + }); + },
98-100: Optional: support non-AWS hosts in base URLHardcoding
.cloud.databricks.comexcludes Azure/GCP hosts. Consider allowing full host values.- _baseUrl() { - return `https://${this.$auth.domain}.cloud.databricks.com/api/2.0`; - }, + _baseUrl() { + const host = this.$auth.domain.includes(".") + ? this.$auth.domain + : `${this.$auth.domain}.cloud.databricks.com`; + return `https://${host}/api/2.0`; + },components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs (1)
19-35: Add pagination support (next_page_token) to return full resultsCurrently returns only the first page.
- async run({ $ }) { - const { vector_indexes = [] } = await this.databricks.listVectorSearchIndexes({ - params: { - endpoint_name: this.endpointName, - }, - $, - }); - - $.export( - "$summary", - `Successfully retrieved ${vector_indexes.length} index${vector_indexes.length === 1 - ? "" - : "es"}.`, - ); - - return vector_indexes; - }, + async run({ $ }) { + const all = []; + let page_token; + do { + const { vector_indexes = [], next_page_token } = + await this.databricks.listVectorSearchIndexes({ + params: { + endpoint_name: this.endpointName, + page_token, + }, + $, + }); + all.push(...vector_indexes); + page_token = next_page_token; + } while (page_token); + + $.export( + "$summary", + `Successfully retrieved ${all.length} index${all.length === 1 ? "" : "es"}.`, + ); + return all; + },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs(1 hunks)components/databricks/actions/list-vector-search-indexes/list-vector-search-indexes.mjs(1 hunks)components/databricks/databricks.app.mjs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/databricks/actions/delete-vector-search-index-data/delete-vector-search-index-data.mjs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Lint Code Base
- GitHub Check: Verify TypeScript components
- GitHub Check: Publish TypeScript components
- GitHub Check: pnpm publish
jcortes
moved this from Changes Required
to Ready for QA
in Component (Source and Action) Backlog
Summary
This PR adds support for Databricks Vector Search Index APIs by implementing 9 new actions.
Changes
Implements 9 Vector Search Index endpoints as discussed in the enhancement issue.
Resolves #18126"
Summary by CodeRabbit
New Features
Chores