feat: add insert rows json function in redshift#300
Merged
Conversation
📝 WalkthroughWalkthroughImplements ChangesRedshift JSON Row Insert
Azure Synapse Datasource Removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Sequence DiagramssequenceDiagram
participant Caller
participant RedshiftClient
participant ColumnQuery
participant InsertCursor
Caller->>RedshiftClient: insert_rows_json(table_name, data)
RedshiftClient->>RedshiftClient: validate data non-empty
RedshiftClient->>RedshiftClient: derive columns from first row
RedshiftClient->>RedshiftClient: validate all rows have identical keys
RedshiftClient->>RedshiftClient: validate table/column identifiers
RedshiftClient->>ColumnQuery: _get_super_columns(table_name)
ColumnQuery-->>RedshiftClient: set of SUPER column names
RedshiftClient->>RedshiftClient: serialize dict/list → JSON, wrap SUPER with JSON_PARSE
RedshiftClient->>InsertCursor: executemany(INSERT INTO ... statement)
alt success
RedshiftClient->>RedshiftClient: connection.commit()
RedshiftClient-->>Caller: return row count
else RedshiftError
RedshiftClient->>RedshiftClient: connection.rollback() + log error
RedshiftClient-->>Caller: raise RedshiftError
end
RedshiftClient->>InsertCursor: close() in finally block
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py`:
- Around line 350-359: The columns are extracted only from the first row using
data[0].keys(), and the serialization logic uses row.get(col) which silently
inserts NULL for missing keys and drops extra keys from later rows without any
validation. To fix this, validate that all rows have a consistent schema
matching the first row before the serialization loop, or collect all unique keys
across all rows first. This ensures data integrity by explicitly handling or
rejecting rows that deviate from the expected schema rather than silently
corrupting data through dropped or missing columns.
- Around line 361-363: The INSERT statement construction is vulnerable to
identifier injection because table_name and column identifiers from the columns
list are directly interpolated into the SQL command string without validation.
Before composing the command variable that constructs the INSERT statement, add
validation logic to ensure that table_name and each column identifier contains
only safe characters (such as alphanumeric characters and underscores) that are
valid for SQL identifiers. This validation should occur before the column_list
and command variables are created to prevent malicious table names or column
names from being used to modify the SQL structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25050d9d-5248-49c3-ba3f-3c76caa876f6
📒 Files selected for processing (2)
wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.pywavefront/server/plugins/datasource/datasource/redshift/__init__.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wavefront/server/modules/plugins_module/plugins_module/services/datasource_services.py (1)
32-41: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftLegacy Synapse datasource records now fail hard without a migration/deprecation path.
With the Synapse branch removed while
DataSourceType.AZURE_SYNAPSEstill exists upstream, persisted Synapse datasources will now fall into the invalid-typeValueErrorpath and fail at runtime. Please add a migration/retirement strategy (or an explicit controlled deprecation branch) before release to avoid request-time breakage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/modules/plugins_module/plugins_module/services/datasource_services.py` around lines 32 - 41, The datasource type validation logic in this conditional chain is missing a case for DataSourceType.AZURE_SYNAPSE, which still exists in persisted datasources upstream despite the Synapse branch being removed. This causes the code to fall through to the ValueError when processing existing Synapse records. Before the final else clause that raises the ValueError, add an explicit elif branch for DataSourceType.AZURE_SYNAPSE that implements a controlled deprecation path, such as logging a deprecation warning and either migrating the datasource to an alternative type (like MSSQL or Postgres) or providing a graceful fallback configuration. This prevents runtime failures for existing Synapse datasource records while providing users with a clear migration/deprecation message.
♻️ Duplicate comments (1)
wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py (1)
390-391: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard against empty column lists.
If the first row is an empty dictionary,
columnswill be an empty list, resulting in malformed SQL:INSERT INTO table () VALUES (). This will cause a runtime error.🛡️ Suggested fix
columns = list(data[0].keys()) +if not columns: + raise ValueError('insert_rows_json requires at least one column') identifier_pattern = re.compile(r'^[A-Za-z_][A-Za-z0-9_]*$')🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py` around lines 390 - 391, The line assigning columns from data[0].keys() does not handle the case where data[0] is an empty dictionary, which results in an empty columns list and generates malformed SQL. Add a guard condition after the columns assignment in the redshift.py file to check if the columns list is empty and raise an appropriate error or handle this edge case before proceeding with SQL generation. This ensures the code does not attempt to create INSERT statements with empty column lists.
🧹 Nitpick comments (2)
wavefront/server/plugins/datasource/datasource/__init__.py (1)
21-22: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMake unsupported Synapse explicit in this boundary.
datasource_typestill accepts the fullDataSourceTypeenum, but this class now supports only four types. Add an explicitAZURE_SYNAPSEbranch (with a deprecation message) or narrow accepted types so failures are deterministic and clearer than the generic invalid-type fallback.Also applies to: 49-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/plugins/datasource/datasource/__init__.py` around lines 21 - 22, The function parameter at the boundary currently accepts the full DataSourceType enum while only supporting four specific types (BigQuery, Redshift, Postgres, MSSQL). Make unsupported types explicit by either narrowing the type hint for the datasource_type parameter to only accept the four supported types using a Literal or Union type annotation, or by adding an explicit branch that catches AZURE_SYNAPSE specifically and raises a clear deprecation error. Apply the same fix to the corresponding location at lines 49-50 to maintain consistency across all boundaries accepting datasource types.wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py (1)
435-435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQuote the table name for consistency and keyword safety.
Column identifiers are quoted on Line [430], but
table_nameis inserted directly without quotes on Line [435]. While the validation (Lines [394-396]) prevents injection, unquoted table/schema names that happen to be SQL keywords (e.g.,order,user,table) will cause syntax errors at runtime.🔧 Recommended fix
+def _quote_qualified_identifier(name: str) -> str: + """Quote each part of a dot-qualified identifier.""" + return '.'.join(f'"{part}"' for part in name.split('.')) + ... - command = f'INSERT INTO {table_name} ({column_list}) VALUES ({placeholders})' + quoted_table = _quote_qualified_identifier(table_name) + command = f'INSERT INTO {quoted_table} ({column_list}) VALUES ({placeholders})'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py` at line 435, The INSERT statement construction at line 435 inserts the `table_name` variable directly without quotes, while column identifiers are quoted elsewhere. To ensure consistency and prevent syntax errors when table names happen to be SQL keywords, add quotes around the `table_name` variable in the INSERT command f-string, matching the quoting approach used for the `column_list`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@wavefront/server/modules/plugins_module/plugins_module/services/datasource_services.py`:
- Around line 24-25: The return type annotation for the function that returns a
tuple of config types does not account for the actual return value of (None,
None). Update the type annotation on the return statement to indicate that both
elements of the tuple can be None by making each config type optional using the
union operator with None or Optional. This change applies to the return type
annotation around line 24-25 and also at line 30, so ensure both locations are
updated to match the actual nullable return contract.
---
Outside diff comments:
In
`@wavefront/server/modules/plugins_module/plugins_module/services/datasource_services.py`:
- Around line 32-41: The datasource type validation logic in this conditional
chain is missing a case for DataSourceType.AZURE_SYNAPSE, which still exists in
persisted datasources upstream despite the Synapse branch being removed. This
causes the code to fall through to the ValueError when processing existing
Synapse records. Before the final else clause that raises the ValueError, add an
explicit elif branch for DataSourceType.AZURE_SYNAPSE that implements a
controlled deprecation path, such as logging a deprecation warning and either
migrating the datasource to an alternative type (like MSSQL or Postgres) or
providing a graceful fallback configuration. This prevents runtime failures for
existing Synapse datasource records while providing users with a clear
migration/deprecation message.
---
Duplicate comments:
In `@wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py`:
- Around line 390-391: The line assigning columns from data[0].keys() does not
handle the case where data[0] is an empty dictionary, which results in an empty
columns list and generates malformed SQL. Add a guard condition after the
columns assignment in the redshift.py file to check if the columns list is empty
and raise an appropriate error or handle this edge case before proceeding with
SQL generation. This ensures the code does not attempt to create INSERT
statements with empty column lists.
---
Nitpick comments:
In `@wavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.py`:
- Line 435: The INSERT statement construction at line 435 inserts the
`table_name` variable directly without quotes, while column identifiers are
quoted elsewhere. To ensure consistency and prevent syntax errors when table
names happen to be SQL keywords, add quotes around the `table_name` variable in
the INSERT command f-string, matching the quoting approach used for the
`column_list`.
In `@wavefront/server/plugins/datasource/datasource/__init__.py`:
- Around line 21-22: The function parameter at the boundary currently accepts
the full DataSourceType enum while only supporting four specific types
(BigQuery, Redshift, Postgres, MSSQL). Make unsupported types explicit by either
narrowing the type hint for the datasource_type parameter to only accept the
four supported types using a Literal or Union type annotation, or by adding an
explicit branch that catches AZURE_SYNAPSE specifically and raises a clear
deprecation error. Apply the same fix to the corresponding location at lines
49-50 to maintain consistency across all boundaries accepting datasource types.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b79ef9f8-588c-458e-8074-5a78c5529ab3
⛔ Files ignored due to path filters (1)
wavefront/server/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
wavefront/server/modules/plugins_module/plugins_module/controllers/datasource_controller.pywavefront/server/modules/plugins_module/plugins_module/services/datasource_services.pywavefront/server/packages/flo_cloud/flo_cloud/aws/redshift.pywavefront/server/packages/flo_cloud/flo_cloud/azure/__init__.pywavefront/server/packages/flo_cloud/flo_cloud/azure/synapse.pywavefront/server/packages/flo_cloud/pyproject.tomlwavefront/server/plugins/datasource/datasource/__init__.pywavefront/server/plugins/datasource/datasource/synapse/__init__.pywavefront/server/plugins/datasource/datasource/synapse/config.py
💤 Files with no reviewable changes (5)
- wavefront/server/plugins/datasource/datasource/synapse/config.py
- wavefront/server/plugins/datasource/datasource/synapse/init.py
- wavefront/server/packages/flo_cloud/flo_cloud/azure/synapse.py
- wavefront/server/packages/flo_cloud/pyproject.toml
- wavefront/server/modules/plugins_module/plugins_module/controllers/datasource_controller.py
Comment on lines
+24
to
25
| BigQueryConfig | RedshiftConfig | PostgresConfig | MSSQLConfig, | ||
| ]: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Return type annotation no longer matches actual return values.
This function can return (None, None) but the annotated tuple currently excludes None for both elements. Please update the signature to reflect the nullable return contract.
Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@wavefront/server/modules/plugins_module/plugins_module/services/datasource_services.py`
around lines 24 - 25, The return type annotation for the function that returns a
tuple of config types does not account for the actual return value of (None,
None). Update the type annotation on the return statement to indicate that both
elements of the tuple can be None by making each config type optional using the
union operator with None or Optional. This change applies to the return type
annotation around line 24-25 and also at line 30, so ensure both locations are
updated to match the actual nullable return contract.
vizsatiz
approved these changes
Jun 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Breaking Changes
Maintenance