feat(ibis): upgrade ibis to canner/10.5.1#1215
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the """ WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Test as test_query_number_scale
participant Client as TestClient
participant API as /query Endpoint
participant DB as Oracle Database
Test->>Client: Prepare manifest & SQL, send POST /query
Client->>API: POST /query with manifest and SQL
API->>DB: Execute SQL on Oracle (SELECT from TestNumber)
DB-->>API: Return row with NUMBER columns
API-->>Client: Respond with data, column types
Client-->>Test: Return response for assertions
Suggested reviewers
Poem
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
github-actions
bot
added
ibis
dependencies
![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 comments (1)
ibis-server/pyproject.toml (1)
13-22: 💡 Verification agent🧩 Analysis chain
Verify git dependency compatibility and consider using HTTPS URL.
The change from a fixed version to a git branch dependency raises several concerns:
- SSH URL compatibility: The
git@github.com:URL requires SSH keys, which may not be available in all deployment environments (CI/CD, Docker builds, etc.)- Stability: Using a branch instead of a tagged release can introduce instability as the branch content may change
- Missing "athena" extra: The removal of the "athena" extra from the list could break existing Athena connector functionality
Consider using an HTTPS URL for broader compatibility:
-ibis-framework = { git = "git@github.com:Canner/ibis.git", branch = "canner/10.5.1", extras = [ +ibis-framework = { git = "https://github.com/Canner/ibis.git", branch = "canner/10.5.1", extras = [Please run the following script to verify the branch exists and check for any athena-related code that might be affected:
🏁 Script executed:
#!/bin/bash # Description: Verify git branch exists and check for athena usage # Check if the branch exists curl -s https://api.github.com/repos/Canner/ibis/branches/canner/10.5.1 | jq '.name // "Branch not found"' # Search for athena-related code in the codebase echo "Searching for athena-related code..." rg -i "athena" --type pyLength of output: 5270
Fix pyproject.toml git dependency and restore Athena extra
The
canner/10.5.1branch exists, but the current change will break Athena support and may cause CI/CD failures:
- The SSH URL (
git@github.com:…) requires SSH keys in build environments- Using a moving branch rather than a tagged release can introduce instability
- Removing
"athena"fromextraswill break all Athena connector code and tests (there are many references underibis-server/tests/…/athena)Please update
ibis-server/pyproject.tomlas follows:- ibis-framework = { git = "git@github.com:Canner/ibis.git", branch = "canner/10.5.1", extras = [ + ibis-framework = { git = "https://github.com/Canner/ibis.git", branch = "canner/10.5.1", extras = [ "bigquery", "clickhouse", + "athena", "mssql", "mysql", "oracle", "postgres", "snowflake", "trino", ] }Optionally, consider pinning to a specific commit or tag for greater stability.
🧹 Nitpick comments (1)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (1)
163-207: Consider extracting manifest for consistency with existing patterns.The test correctly validates Oracle NUMBER type handling with precision and scale. The assertions for both data values
[1, 1234567890, "12345678.12"]and data types are appropriate, especially verifying that the scaled number column (id_p_s) returns as"object"type.For consistency with other tests in this file, consider extracting the manifest definition:
+test_number_manifest = { + "catalog": "my_catalog", + "schema": "my_schema", + "dataSource": "oracle", + "models": [ + { + "name": "TestNumber", + "tableReference": { + "schema": "SYSTEM", + "table": "TEST_NUMBER", + }, + "columns": [ + {"name": "id", "expression": '"ID"', "type": "number"}, + {"name": "id_p", "expression": '"ID_P"', "type": "number"}, + {"name": "id_p_s", "expression": '"ID_P_S"', "type": "number"}, + ], + "primaryKey": "id", + } + ], +} + async def test_query_number_scale(client, connection_info): - manifest_str = { - "catalog": "my_catalog", - "schema": "my_schema", - "dataSource": "oracle", - "models": [ - { - "name": "TestNumber", - "tableReference": { - "schema": "SYSTEM", - "table": "TEST_NUMBER", - }, - "columns": [ - {"name": "id", "expression": '"ID"', "type": "number"}, - {"name": "id_p", "expression": '"ID_P"', "type": "number"}, - {"name": "id_p_s", "expression": '"ID_P_S"', "type": "number"}, - ], - "primaryKey": "id", - } - ], - } - - manifest_str_base64 = base64.b64encode(orjson.dumps(manifest_str)).decode("utf-8") + manifest_str_base64 = base64.b64encode(orjson.dumps(test_number_manifest)).decode("utf-8")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ibis-server/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
ibis-server/pyproject.toml(1 hunks)ibis-server/tests/routers/v3/connector/oracle/conftest.py(1 hunks)ibis-server/tests/routers/v3/connector/oracle/test_query.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ibis-server/tests/routers/v3/connector/oracle/test_query.py (3)
ibis-server/tests/conftest.py (1)
client(18-23)ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
connection_info(68-77)ibis-server/tests/routers/v3/connector/oracle/test_function.py (1)
manifest_str(31-32)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (1)
ibis-server/tests/routers/v3/connector/oracle/conftest.py (1)
52-62: Well-structured test infrastructure for Oracle NUMBER types.The addition of the
test_numbertable with different NUMBER column configurations (generic NUMBER, NUMBER(10), and NUMBER(10,2)) provides comprehensive test coverage for Oracle's numeric precision and scale handling. The test data values are well-chosen to validate the different scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/ibis-ci.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
goldmedal
force-pushed
the
chore/self-host-ibis-10.5.1
branch
from
4d9942a to
57fdb7a
Compare
| pydantic = "2.10.6" | ||
| ibis-framework = { version = "10.3.1", extras = [ | ||
| "athena", | ||
| ibis-framework = { git = "https://github.com/Canner/ibis.git", branch = "canner/10.5.1", extras = [ |
There was a problem hiding this comment.
Seems like 'athena' was mistakenly deleted?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ibis-server/pyproject.toml (1)
14-23: Remove or justify theathenaextra
The summary noted removing"athena", yet it remains in the extras list. If Athena support isn’t needed in this upgrade, drop the entry; otherwise confirm its inclusion.- "athena",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ibis-server/pyproject.toml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
|
Thanks @goldmedal |
2 participants
Summary by CodeRabbit
Chores
Tests