fix: create or drop view #349
Conversation
WalkthroughThe pull request bumps the package version to 0.9.8 and introduces view management utilities across multiple datasource integrations. New methods enable dynamic SQL view creation, deletion, and name generation for SQL, BigQuery, and Oracle datasources, supporting both SQLAlchemy and native connections with error handling. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings, 1 inconclusive)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
🧹 Nitpick comments (2)
dcs_core/core/datasource/sql_datasource.py (1)
1053-1136: View helpers are generally solid; consider small API & robustness tweaksThe new
generate_view_name,create_view, anddrop_viewimplementations look correct and cover SQLAlchemyEngine/Connectionas well as plain DB‑API style connections. A couple of small improvements to consider:
- Guard against missing connection explicitly
Right now, if
self.connectionisNone, the logic falls through to the generic branch and eventually hitsself.connection.execute(...), yielding anAttributeErrorthat gets caught and logged as a generic “Error creating/dropping view …” message.You could fail fast with a clearer error:
def create_view( self, query: str | None = None, schema: str | None = None, view_name: str | None = None, ) -> str | None: + if self.connection is None: + logger.error("Cannot create view because no connection is established.") + return Noneand similarly for
drop_view.
- Make
schemaoptional indrop_viewfor API symmetry
create_viewtreatsschemaas optional, butdrop_viewcurrently requires it:def drop_view(self, view_name: str, schema: str | None) -> bool:For a smoother caller experience and to mirror
create_view, you might want:- def drop_view(self, view_name: str, schema: str | None) -> bool: + def drop_view(self, view_name: str, schema: str | None = None) -> bool:so callers don’t have to remember different defaults between create and drop.
dcs_core/integrations/databases/oracle.py (1)
15-17: Oraclegenerate_view_namebehavior is correct; minor cleanup possibleThe override correctly enforces uppercase view names for Oracle, and the generated name length stays within Oracle’s identifier limits. One small readability tweak:
random_string = "".join( secrets.choice(string.ascii_letters + string.digits) for _ in range(8) ) timestamp = int(time.time()) return f"dcs_view_{timestamp}_{random_string.lower()}".upper()The
.lower()followed by.upper()on the whole string is redundant. You could simplify to:- random_string = "".join( - secrets.choice(string.ascii_letters + string.digits) for _ in range(8) - ) - timestamp = int(time.time()) - return f"dcs_view_{timestamp}_{random_string.lower()}".upper() + random_string = "".join( + secrets.choice(string.ascii_letters + string.digits) for _ in range(8) + ) + timestamp = int(time.time()) + return f"DCS_VIEW_{timestamp}_{random_string}"Functionality stays the same but the intent is clearer.
Also applies to: 697-704
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
dcs_core/__version__.py(1 hunks)dcs_core/core/datasource/sql_datasource.py(2 hunks)dcs_core/integrations/databases/bigquery.py(2 hunks)dcs_core/integrations/databases/oracle.py(2 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
dcs_core/core/datasource/sql_datasource.py (4)
dcs_core/integrations/databases/oracle.py (2)
generate_view_name(697-704)connect(47-71)dcs_core/integrations/databases/bigquery.py (3)
create_view(166-191)connect(37-75)drop_view(193-205)dcs_core/integrations/databases/mysql.py (1)
connect(43-74)dcs_core/integrations/databases/postgres.py (1)
connect(33-57)
dcs_core/integrations/databases/bigquery.py (2)
dcs_core/core/datasource/sql_datasource.py (3)
create_view(1063-1105)generate_view_name(1054-1061)drop_view(1107-1136)dcs_core/integrations/databases/oracle.py (1)
generate_view_name(697-704)
dcs_core/integrations/databases/oracle.py (1)
dcs_core/core/datasource/sql_datasource.py (1)
generate_view_name(1054-1061)
🪛 Ruff (0.14.5)
dcs_core/core/datasource/sql_datasource.py
1088-1089: try-except-pass detected, consider logging the exception
(S110)
1088-1088: Do not catch blind exception: Exception
(BLE001)
1097-1098: try-except-pass detected, consider logging the exception
(S110)
1097-1097: Do not catch blind exception: Exception
(BLE001)
1102-1102: Consider moving this statement to an else block
(TRY300)
1103-1103: Do not catch blind exception: Exception
(BLE001)
1121-1122: try-except-pass detected, consider logging the exception
(S110)
1121-1121: Do not catch blind exception: Exception
(BLE001)
1129-1130: try-except-pass detected, consider logging the exception
(S110)
1129-1129: Do not catch blind exception: Exception
(BLE001)
1133-1133: Consider moving this statement to an else block
(TRY300)
1134-1134: Do not catch blind exception: Exception
(BLE001)
dcs_core/integrations/databases/bigquery.py
189-189: Do not catch blind exception: Exception
(BLE001)
202-202: Consider moving this statement to an else block
(TRY300)
203-203: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
pyproject.toml (1)
3-3: Version bump looks consistentVersion updated to
0.9.8here and matchesdcs_core/__version__.__version__; no issues from this change.dcs_core/__version__.py (1)
15-15: Public version constant correctly updated
__version__ = "0.9.8"matches the project metadata; this is good to go.
| def create_view( | ||
| self, | ||
| query: Optional[str] = None, | ||
| dataset: Optional[str] = None, | ||
| view_name: Optional[str] = None, | ||
| ) -> str | None: | ||
| view_name = self.generate_view_name(view_name=view_name) | ||
| full_name = ( | ||
| f"`{self.project}`.`{dataset}`.`{view_name}`" | ||
| if dataset | ||
| else f"`{view_name}`" | ||
| ) | ||
| try: | ||
| if query is None: | ||
| create_view_query = ( | ||
| f"CREATE VIEW {full_name} AS SELECT 1 AS dummy_column WHERE FALSE" | ||
| ) | ||
| self.connection.execute(create_view_query) | ||
| return full_name | ||
| else: | ||
| create_view_query = f"CREATE VIEW {full_name} AS {query}" | ||
| self.connection.execute(create_view_query) | ||
| return full_name | ||
| except Exception as e: | ||
| logger.error(f"Error creating view: {e}") | ||
| return None | ||
|
|
||
| def drop_view(self, view_name: str, dataset: Optional[str] = None) -> bool: | ||
| full_name = ( | ||
| f"`{self.project}`.`{dataset}`.`{view_name}`" | ||
| if dataset | ||
| else f"`{view_name}`" | ||
| ) | ||
| try: | ||
| drop_view_query = f"DROP VIEW {full_name}" | ||
| self.connection.execute(drop_view_query) | ||
| return True | ||
| except Exception as e: | ||
| logger.error(f"Error dropping view: {e}") | ||
| return False |
There was a problem hiding this comment.
Bug: self.project does not exist; use self.project_id (and optionally default dataset)
In both create_view and drop_view, full_name is built using self.project:
full_name = (
f"`{self.project}`.`{dataset}`.`{view_name}`"
if dataset
else f"`{view_name}`"
)But in __init__, the attribute is named self.project_id, not self.project:
self.project_id = self.data_connection.get("project")At runtime this will raise AttributeError: 'BigQueryDataSource' object has no attribute 'project', causing all view operations to fail.
You likely want:
- view_name = self.generate_view_name(view_name=view_name)
- full_name = (
- f"`{self.project}`.`{dataset}`.`{view_name}`"
- if dataset
- else f"`{view_name}`"
- )
+ view_name = self.generate_view_name(view_name=view_name)
+ dataset = dataset or self.dataset_id
+ full_name = (
+ f"`{self.project_id}`.`{dataset}`.`{view_name}`"
+ if dataset
+ else f"`{view_name}`"
+ )and similarly for drop_view:
- full_name = (
- f"`{self.project}`.`{dataset}`.`{view_name}`"
- if dataset
- else f"`{view_name}`"
- )
+ dataset = dataset or self.dataset_id
+ full_name = (
+ f"`{self.project_id}`.`{dataset}`.`{view_name}`"
+ if dataset
+ else f"`{view_name}`"
+ )This both fixes the missing‑attribute bug and makes the optional dataset argument fall back to the configured default, which is more ergonomic for callers.
🧰 Tools
🪛 Ruff (0.14.5)
189-189: Do not catch blind exception: Exception
(BLE001)
202-202: Consider moving this statement to an else block
(TRY300)
203-203: Do not catch blind exception: Exception
(BLE001)
2 participants
Summary by CodeRabbit
Version 0.9.8
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.