Skip to content

[DATABRICKS-4] PLU-600: add databricks create row action#1529

Open
pregnantboy wants to merge 1 commit intodatabricks/3-dynamic-datafrom
databricks/4-create-row
Open

[DATABRICKS-4] PLU-600: add databricks create row action#1529
pregnantboy wants to merge 1 commit intodatabricks/3-dynamic-datafrom
databricks/4-create-row

Conversation

@pregnantboy
Copy link
Copy Markdown
Contributor

@pregnantboy pregnantboy commented Apr 7, 2026

Add create-row action for Databricks integration

Add create-row action with zod validation and parameterized SQL insert.
Supports dynamic table/column selection via dropdowns with inline
create-new-column and modal create-new-table options.

The action validates table and column names to only allow lowercase letters, numbers, and underscores. It constructs INSERT statements with named parameters for safe SQL execution and includes comprehensive error handling with structured logging.

Copy link
Copy Markdown
Contributor Author

pregnantboy commented Apr 7, 2026

@pregnantboy pregnantboy changed the title feat: add databricks create row action [DATABRICKS-4] PLU-600: add databricks create row action Apr 7, 2026
@linear
Copy link
Copy Markdown

linear bot commented Apr 7, 2026

@pregnantboy pregnantboy marked this pull request as ready for review April 7, 2026 07:01
@pregnantboy pregnantboy requested a review from a team as a code owner April 7, 2026 07:01
@datadog-opengovsg
Copy link
Copy Markdown

datadog-opengovsg bot commented Apr 7, 2026

✅ Tests

🎉 All green!

❄️ No new flaky tests detected
🧪 All tests passed

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 775b175 | Docs | Was this helpful? Give us feedback!

Comment thread packages/backend/src/apps/databricks/actions/create-row.ts Outdated
Comment thread packages/backend/src/apps/databricks/actions/create-row.ts
@pregnantboy pregnantboy force-pushed the databricks/3-dynamic-data branch from 9df0e2b to 23f55f5 Compare April 7, 2026 09:43
@pregnantboy pregnantboy force-pushed the databricks/4-create-row branch 2 times, most recently from 189d3bc to f94b3fa Compare April 7, 2026 09:51
Comment on lines +21 to +32
rowData: z.array(
z.object({
columnName: z
.string()
.min(1, { message: 'Column name is required' })
.regex(/^[a-z0-9_]+$/, {
message:
'Column name can only contain lowercase letters, numbers and underscores',
}),
columnValue: z.string(),
}),
),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rowData array schema has no minimum length validation, allowing empty arrays. This will generate invalid SQL like INSERT INTO table () VALUES () and cause execution to fail.

Fix by adding minimum length validation:

rowData: z.array(
  z.object({
    columnName: z
      .string()
      .min(1, { message: 'Column name is required' })
      .regex(/^[a-z0-9_]+$/, {
        message:
          'Column name can only contain lowercase letters, numbers and underscores',
      }),
    columnValue: z.string(),
  }),
).min(1, { message: 'At least one column is required' }),
Suggested change
rowData: z.array(
z.object({
columnName: z
.string()
.min(1, { message: 'Column name is required' })
.regex(/^[a-z0-9_]+$/, {
message:
'Column name can only contain lowercase letters, numbers and underscores',
}),
columnValue: z.string(),
}),
),
rowData: z.array(
z.object({
columnName: z
.string()
.min(1, { message: 'Column name is required' })
.regex(/^[a-z0-9_]+$/, {
message:
'Column name can only contain lowercase letters, numbers and underscores',
}),
columnValue: z.string(),
}),
).min(1, { message: 'At least one column is required' }),

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlikely to happen but good to have

@pregnantboy pregnantboy force-pushed the databricks/3-dynamic-data branch from 23f55f5 to 6e6fe2e Compare April 13, 2026 03:08
@pregnantboy pregnantboy force-pushed the databricks/4-create-row branch from f94b3fa to 6ee411e Compare April 13, 2026 03:08
@pregnantboy pregnantboy force-pushed the databricks/3-dynamic-data branch from 6e6fe2e to 62c5d53 Compare April 13, 2026 03:30
@pregnantboy pregnantboy force-pushed the databricks/4-create-row branch from 6ee411e to 41da125 Compare April 13, 2026 03:30
Comment on lines +126 to +144
const { session, endSession } = await createSession($)
const { tableName, rowData } = parametersParseResult.data
const columnNames = rowData.map((row) => row.columnName)
const columnValues = rowData.map((row) => row.columnValue)

const statement = `INSERT INTO \`${tableName}\` (${columnNames
.map((col) => `\`${col}\``)
.join(',')}) VALUES (${columnValues
.map((_val, index) => `:val${index}`)
.join(',')})`
const namedParameters = {} as Record<string, string>
for (let i = 0; i < columnValues.length; i++) {
namedParameters[`val${i}`] = columnValues[i]
}
const operation = await session.executeStatement(statement, {
namedParameters,
})
await operation.fetchAll()
await endSession()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource leak: if an error occurs during statement execution or fetchAll (lines 140-143), the session created on line 126 is never closed, as endSession() on line 144 is skipped when the catch block is entered. This will leak Databricks sessions in production.

Fix by ensuring cleanup:

const { session, endSession } = await createSession($)
try {
  const { tableName, rowData } = parametersParseResult.data
  // ... SQL execution ...
  await operation.fetchAll()
} finally {
  await endSession()
}
Suggested change
const { session, endSession } = await createSession($)
const { tableName, rowData } = parametersParseResult.data
const columnNames = rowData.map((row) => row.columnName)
const columnValues = rowData.map((row) => row.columnValue)
const statement = `INSERT INTO \`${tableName}\` (${columnNames
.map((col) => `\`${col}\``)
.join(',')}) VALUES (${columnValues
.map((_val, index) => `:val${index}`)
.join(',')})`
const namedParameters = {} as Record<string, string>
for (let i = 0; i < columnValues.length; i++) {
namedParameters[`val${i}`] = columnValues[i]
}
const operation = await session.executeStatement(statement, {
namedParameters,
})
await operation.fetchAll()
await endSession()
const { session, endSession } = await createSession($)
try {
const { tableName, rowData } = parametersParseResult.data
const columnNames = rowData.map((row) => row.columnName)
const columnValues = rowData.map((row) => row.columnValue)
const statement = `INSERT INTO \`${tableName}\` (${columnNames
.map((col) => `\`${col}\``)
.join(',')}) VALUES (${columnValues
.map((_val, index) => `:val${index}`)
.join(',')})`
const namedParameters = {} as Record<string, string>
for (let i = 0; i < columnValues.length; i++) {
namedParameters[`val${i}`] = columnValues[i]
}
const operation = await session.executeStatement(statement, {
namedParameters,
})
await operation.fetchAll()
} finally {
await endSession()
}

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems valid. if somehow createSession fails, it will still be caught and thrown anw

Copy link
Copy Markdown
Contributor

@kevinkim-ogp kevinkim-ogp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, can create row successfully via the action!

probably can address the two comments from Graphite, both seem valid.

sidenote: users can manually upload datasets to databricks with invalid column names i.e., with spaces, and our action will fail. doesn't seem like we have much control over this, but probably good to include somewhere in our guide

Comment on lines +21 to +32
rowData: z.array(
z.object({
columnName: z
.string()
.min(1, { message: 'Column name is required' })
.regex(/^[a-z0-9_]+$/, {
message:
'Column name can only contain lowercase letters, numbers and underscores',
}),
columnValue: z.string(),
}),
),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlikely to happen but good to have

Comment on lines +126 to +144
const { session, endSession } = await createSession($)
const { tableName, rowData } = parametersParseResult.data
const columnNames = rowData.map((row) => row.columnName)
const columnValues = rowData.map((row) => row.columnValue)

const statement = `INSERT INTO \`${tableName}\` (${columnNames
.map((col) => `\`${col}\``)
.join(',')}) VALUES (${columnValues
.map((_val, index) => `:val${index}`)
.join(',')})`
const namedParameters = {} as Record<string, string>
for (let i = 0; i < columnValues.length; i++) {
namedParameters[`val${i}`] = columnValues[i]
}
const operation = await session.executeStatement(statement, {
namedParameters,
})
await operation.fetchAll()
await endSession()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems valid. if somehow createSession fails, it will still be caught and thrown anw

@pregnantboy pregnantboy force-pushed the databricks/3-dynamic-data branch from 62c5d53 to e8e971b Compare April 16, 2026 08:44
@pregnantboy pregnantboy force-pushed the databricks/4-create-row branch from 41da125 to a3de2ea Compare April 16, 2026 08:44
Add create-row action with zod validation and parameterized SQL insert.
Supports dynamic table/column selection via dropdowns with inline
create-new-column and modal create-new-table options.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@pregnantboy pregnantboy force-pushed the databricks/3-dynamic-data branch from e8e971b to ea3ac20 Compare April 17, 2026 06:32
@pregnantboy pregnantboy force-pushed the databricks/4-create-row branch from a3de2ea to 775b175 Compare April 17, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants