Fix: Normalize schema names for correct name comparison#296
Conversation
WalkthroughWalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SQLProcessor
participant Normalizer
Note over SQLProcessor: Before the change
User->>SQLProcessor: Call _ensure_schema_exists()
SQLProcessor->>SQLProcessor: Get schema name from config
SQLProcessor->>SQLProcessor: Check schema existence
Note over SQLProcessor: After the change
User->>SQLProcessor: Call _ensure_schema_exists()
SQLProcessor->>Normalizer: Normalize schema name
Normalizer-->>SQLProcessor: Return normalized schema name
SQLProcessor->>Normalizer: Normalize existing schemas list
Normalizer-->>SQLProcessor: Return normalized schemas list
SQLProcessor->>SQLProcessor: Check schema existence
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
|
CodeRabbit (@coderabbitai) review |
Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- airbyte/_future_cdk/sql_processor.py (1 hunks)
Additional comments not posted (1)
airbyte/_future_cdk/sql_processor.py (1)
306-307: LGTM! But verify the normalization method usage in the codebase.The code changes are approved.
However, ensure that the
normalize_namemethod from the SQL engine dialect is used correctly across the codebase.Verification successful
The
normalize_namemethod is used correctly and only in the intended context.
- The
normalize_namemethod from the SQL engine dialect is used only in the_ensure_schema_existsmethod in theairbyte/_future_cdk/sql_processor.pyfile and is implemented correctly.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the normalize_name method from the SQL engine dialect. # Test: Search for the usage of normalize_name method. Expect: Only occurances of the correct usage. rg --type python -A 5 $'normalize_name'Length of output: 69
Script:
#!/bin/bash # Description: Verify the usage of the normalize_name method from the SQL engine dialect. # Test: Search for the usage of normalize_name method. Expect: Only occurrences of the correct usage. rg --glob '*.py' -A 5 'normalize_name'Length of output: 466
|
nakamichi (@nakamichiworks) - Thanks for submitting this! Can you clarify for me if the issue is related to casing or any other specific normalization issue? For context on casing, we have an internal |
|
Thanks for your review and sorry for a late reply.
However, it might be best to define and use something like |
|
/test-pr
|
Thank you for this suggestion. I am a bit wary to adopt Snowflake's own case normalization because some string casings are sometimes-unreachable given Snowflake's odd "case-unaware, case-sensitive" handling. Instead, we try to exclusively interface with Snowflake using the QUOTED_IDENTIFIERS_IGNORE_CASE setting enabled - expecting all-lowercase identifiers in Python and all-uppercase when it comes to Snowflake's internal handling. Since MIxeD case identifiers are sometimes unreachable, we essentially disallow them in PyAirbyte-managed tables/schemas/columns. It's not a perfect solution, per se, and we will certainly consider adapting the internal dialect version in the future. 👍 |
|
/test-pr
Edit: Failure is false-positive. All checks successful! 🙌 ✅ |
2 participants
Fixes #279.
Since sqlalchemy always returns normalized schema names, user given schema name must be also normalized for correct comparison.
Summary by CodeRabbit