Chore: Bump CDK dependency, DuckDB, Postgres, Pandas, and others #383
Conversation
WalkthroughWalkthroughThe pull request updates the Changes
Possibly related PRs
Would you like to add any additional context or details to the summary? wdyt? Recent review detailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (4)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (3)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
aaronsteers
changed the title
![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
Outside diff range and nitpick comments (2)
airbyte/shared/state_providers.py (1)
13-13: LGTM! Nice import consolidation.The change looks good and aligns well with the other imports from
airbyte_protocol.models. It simplifies the import structure without affecting functionality.Quick thought: Since we're importing multiple items from
airbyte_protocol.models, what do you think about using a single import statement with multiple items? Something like this:from airbyte_protocol.models import ( AirbyteStateMessage, AirbyteStateType, AirbyteStreamState, )This could make the imports even cleaner. Wdyt? 🤔
airbyte/sources/base.py (1)
42-42: LGTM! Simplified import statement.The change looks good! It simplifies the import by removing the redundant
airbyte_protocolmodule specification. This improves readability without affecting functionality.Quick question: Have you considered using absolute imports throughout the project for consistency? For example:
from airbyte_protocol.models import AirbyteStream. Wdyt?
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
Files selected for processing (8)
- airbyte/_processors/sql/postgres.py (1 hunks)
- airbyte/caches/_state_backend_base.py (1 hunks)
- airbyte/datasets/_sql.py (1 hunks)
- airbyte/shared/state_providers.py (1 hunks)
- airbyte/shared/state_writers.py (1 hunks)
- airbyte/sources/base.py (1 hunks)
- pyproject.toml (4 hunks)
- tests/conftest.py (1 hunks)
Files skipped from review due to trivial changes (4)
- airbyte/caches/_state_backend_base.py
- airbyte/datasets/_sql.py
- airbyte/shared/state_writers.py
- tests/conftest.py
Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
Additional comments not posted (2)
airbyte/_processors/sql/postgres.py (1)
32-32: Interesting change to the SQLAlchemy connection string! 🤔I noticed you've removed the
+psycopg2part from the connection string. This change simplifies the URL, which is cool! But I'm curious about a couple of things:
- Are we still using
psycopg2as the database driver behind the scenes? If so, removing this explicit mention might lead to some ambiguity.- Have we tested this change thoroughly to ensure it doesn't affect any existing connections or queries?
What do you think about adding a comment explaining the rationale behind this change? It could help future maintainers understand why we're using this specific format. wdyt?
To ensure this change doesn't introduce any issues, we could run a quick check:
This will help us verify if we need to update any other parts of the codebase or if psycopg2 is still a dependency we're using.
Verification successful
Verified:
psycopg2has been successfully removed! 🎉Great job simplifying the SQLAlchemy connection string by removing
+psycopg2. Since there are no remaining references topostgresql+psycopg2andpsycopg2isn't listed as a dependency, this change looks safe.Maybe consider adding a comment to explain the rationale behind this update for future maintainers. What do you think? 😊
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to postgresql+psycopg2 in the codebase # Search for any remaining references to postgresql+psycopg2 rg "postgresql\+psycopg2" --type python # Check if psycopg2 is still a dependency grep "psycopg2" pyproject.tomlLength of output: 103
Script:
#!/bin/bash # Description: Corrected script to search for 'postgresql+psycopg2' and verify psycopg2 dependency # Search for any remaining references to postgresql+psycopg2 in Python files rg "postgresql\+psycopg2" --type py # Check if psycopg2 is still a dependency in pyproject.toml grep "psycopg2" pyproject.tomlLength of output: 68
Script:
#!/bin/bash # Description: Search for 'psycopg2' in common dependency files # Define an array of potential dependency files DEPENDENCY_FILES=("requirements.txt" "setup.py" "Pipfile" "environment.yml") # Iterate over each file and search for 'psycopg2' for file in "${DEPENDENCY_FILES[@]}"; do if [ -f "$file" ]; then echo "Checking $file for 'psycopg2'..." rg "psycopg2" "$file" else echo "$file does not exist." fi doneLength of output: 600
airbyte/shared/state_providers.py (1)
Line range hint
1-180: Verified: AirbyteStreamState usage is consistentI've checked the usage of
AirbyteStreamStatethroughout the file, and it's all good! The import change doesn't affect any of the existing code. Everything's still working as expected.Just a friendly reminder: If you're making similar import changes in other files, it might be worth double-checking those too. But for this file, we're golden! 👍
aaronsteers
changed the title
aaronsteers
changed the title
|
Note for follow up... To use the latest Postgres driver (v3), we need to use the following driver specification in the SQLalchemy URL: "postgresql+psycopg" |
Summary by CodeRabbit
New Features
Bug Fixes
psycopgand Snowflake connectors.Chores