chore(core): merge release/0.14.7 back to main branch #1125
Conversation
WalkthroughThis pull request introduces a new documentation section in the README for reporting migration issues along with detailed instructions. It updates the conditional logic for configuring data sources in the server, marks several v2 API endpoints as deprecated, and adds a new dependency injection and error fallback mechanism in the v3 connector. In addition, the version numbers in configuration and Maven files have been updated, and new tests have been added or adjusted for functionality and error message assertions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant V3Connector as V3 Connector
participant JEngine as JavaEngineConnector
participant V2Connector as V2 Connector
Client->>V3Connector: Sends Query Request
V3Connector->>JEngine: Execute Query
alt Query Succeeds
JEngine-->>V3Connector: Return Query Result
V3Connector-->>Client: Return Result
else Query Fails
V3Connector->>V2Connector: Fallback to v2 Query Execution
V2Connector-->>V3Connector: Return Fallback Result
V3Connector-->>Client: Return Fallback Result (with MIGRATION_MESSAGE)
end
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 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 (
|
github-actions
bot
added
documentation
![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/app/routers/v3/connector.py (1)
160-187: 🛠️ Refactor suggestionMissing migration message in model-substitute fallback log
The fallback log message for the model-substitute endpoint is missing the MIGRATION_MESSAGE constant that is included in all other fallback implementations.
- logger.warning( - "Failed to execute v3 model-substitute, fallback to v2: {}", str(e) - ) + logger.warning( + "Failed to execute v3 model-substitute, fallback to v2: {}\n" + MIGRATION_MESSAGE, + str(e) + )
🧹 Nitpick comments (6)
ibis-server/app/config.py (1)
64-65: Standardization of file-related data sourcesThe implementation maps multiple file-based data sources to use the "duckdb" function list, which improves consistency in function handling.
Consider adding a comment explaining why these specific data sources are mapped to "duckdb" to make the intention clear for future maintainers:
if data_source in {"local_file", "s3_file", "minio_file", "gcs_file"}: + # All file-based data sources use DuckDB for processing functions data_source = "duckdb"Also consider adding type checking or validation for the
data_sourceparameter to handle potential edge cases like None values.ibis-server/README.md (2)
94-117: Great addition of documentation for migration reporting!This new section clearly explains the migration process to v3 API and provides detailed instructions for users to report issues. The example log message and structured reporting steps will help collect valuable feedback for improving the migration process.
To enhance this section:
- Consider adding "the" before "Wren engine" in line 95: "The Wren engine is migrating..."
- Ensure log example reflects actual format. If this is a mockup, add a note clarifying that the timestamps and IDs are examples.
-Wren engine is migrating to v3 API (powered by Rust and DataFusion). However, there are some SQL issues currently. +The Wren engine is migrating to v3 API (powered by Rust and DataFusion). However, there are some SQL issues currently.🧰 Tools
🪛 LanguageTool
[uncategorized] ~95-~95: You might be missing the article “the” here.
Context: ...ation Issue Wren engine is migrating to v3 API (powered by Rust and DataFusion). H...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~117-~117: Consider using a different verb for a more formal wording.
Context: ...ed information helps us to diagnose and fix the issues more efficiently. Thank you ...(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.17.2)
100-100: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
100-108: Add language specifier to the code blockAdding a language specifier to the fenced code block improves syntax highlighting and readability.
-``` +```log 2025-03-19 22:49:08.788 | [62781772-7120-4482-b7ca-4be65c8fda96] | INFO | __init__.dispatch:14 - POST /v3/connector/postgres/query ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
100-100: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
ibis-server/app/routers/v3/connector.py (1)
30-31: Address grammar in migration messageThe migration message contains a grammatical error that should be fixed.
-MIGRATION_MESSAGE = "Wren engine is migrating to Rust version now. \ - Wren AI team are appreciate if you can provide the error messages and related logs for us." +MIGRATION_MESSAGE = "Wren engine is migrating to Rust version now. \ + Wren AI team would appreciate if you can provide the error messages and related logs for us."ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (1)
28-95: Comprehensive test coverage for fallback scenariosThe test suite thoroughly covers all fallback scenarios including query, dry run, dry plan, dry plan for data source, and validate. All assertions are clear and meaningful.
A small enhancement to consider is adding a negative test case that verifies the error response when the fallback to v2 also fails (perhaps by deliberately introducing invalid connection info), to ensure error propagation works as expected.
ibis-server/app/routers/v2/connector.py (1)
33-165: Consider adding migration guidance for deprecated endpoints.While all v2 endpoints have been consistently marked as deprecated, consider adding comments or documentation references to guide users on which v3 endpoints they should migrate to. This would make the migration path clearer for API consumers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
ibis-server/resources/function_list/duckdb.csvis excluded by!**/*.csv
📒 Files selected for processing (16)
ibis-server/README.md(1 hunks)ibis-server/app/config.py(1 hunks)ibis-server/app/routers/v2/analysis.py(1 hunks)ibis-server/app/routers/v2/connector.py(7 hunks)ibis-server/app/routers/v3/connector.py(6 hunks)ibis-server/pyproject.toml(1 hunks)ibis-server/tests/routers/v3/connector/local_file/test_functions.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py(1 hunks)ibis-server/tests/routers/v3/connector/postgres/test_model_substitute.py(0 hunks)ibis-server/tests/routers/v3/connector/postgres/test_query.py(0 hunks)pom.xml(1 hunks)trino-parser/pom.xml(1 hunks)wren-base/pom.xml(1 hunks)wren-main/pom.xml(1 hunks)wren-server/pom.xml(1 hunks)wren-tests/pom.xml(1 hunks)
💤 Files with no reviewable changes (2)
- ibis-server/tests/routers/v3/connector/postgres/test_query.py
- ibis-server/tests/routers/v3/connector/postgres/test_model_substitute.py
🧰 Additional context used
🧬 Code Definitions (3)
ibis-server/app/routers/v3/connector.py (2)
ibis-server/app/routers/v2/connector.py (6)
get_java_engine_connector(29-30)query(36-65)dry_plan(134-144)dry_plan_for_data_source(148-162)validate(69-89)model_substitute(166-186)ibis-server/app/model/__init__.py (1)
DryPlanDTO(247-249)
ibis-server/tests/routers/v3/connector/local_file/test_functions.py (1)
ibis-server/app/config.py (2)
get_config(79-80)set_remote_function_list_path(72-73)
ibis-server/app/routers/v2/connector.py (1)
ibis-server/app/model/metadata/factory.py (2)
MetadataFactory(36-42)get_metadata(38-42)
🪛 LanguageTool
ibis-server/README.md
[uncategorized] ~95-~95: You might be missing the article “the” here.
Context: ...ation Issue Wren engine is migrating to v3 API (powered by Rust and DataFusion). H...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[style] ~117-~117: Consider using a different verb for a more formal wording.
Context: ...ed information helps us to diagnose and fix the issues more efficiently. Thank you ...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.17.2)
ibis-server/README.md
100-100: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (26)
pom.xml (1)
13-13: Version Bump Update in Root POM
The project version has been updated from0.14.7-SNAPSHOTto0.14.10-SNAPSHOT. This change is consistent with the coordinated version bumps across the modules and aligns with the upgrade mentioned in the PR description. Please ensure that your CI/CD pipelines and any downstream consumers are aware of this new version.wren-base/pom.xml (1)
21-21: Parent Version Update for wren-base
The parent artifact version has been updated to0.14.10-SNAPSHOTfrom0.14.7-SNAPSHOT. The change is straightforward and appears consistent with the overall version bump strategy across the project modules.wren-main/pom.xml (1)
8-8: Parent Version Update in wren-main
The parent version is now set to0.14.10-SNAPSHOT, ensuring that wren-main is built against the latest parent version. This update is consistent with similar changes in other modules and helps maintain uniform versioning across the codebase.wren-server/pom.xml (1)
8-8: Parent Version Bump in wren-server
The updated parent version (0.14.10-SNAPSHOT) in wren-server aligns perfectly with the changes in the other modules. Additionally, the configuration properties (including the main-class setup) appear intact.trino-parser/pom.xml (1)
8-8: Consistent Version Update in trino-parser
The parent version for trino-parser has been bumped to0.14.10-SNAPSHOT. This ensures that all modules, including trino-parser, rely on a unified version of the parent artifact. The version bump is well executed and consistent with the overall release strategy.wren-tests/pom.xml (1)
21-21: Version update as part of release mergeThe version has been updated from
0.14.7-SNAPSHOTto0.14.10-SNAPSHOTas part of merging the release branch back to main, which is consistent with the PR objectives.ibis-server/pyproject.toml (1)
3-3: Version increment aligned with Maven versioningThe version has been updated from
0.14.6to0.14.9in the Python package, which aligns with the version updates in the Maven files but uses a slightly different convention (no-SNAPSHOTsuffix).ibis-server/app/routers/v2/analysis.py (2)
14-14: API endpoint properly marked as deprecatedGood practice to mark the v2 batch endpoint as deprecated rather than removing it abruptly, allowing users time to migrate to newer API versions.
9-9:❓ Verification inconclusive
API endpoint properly marked as deprecated
Good practice to mark the v2 endpoint as deprecated rather than removing it abruptly, allowing users time to migrate to newer API versions.
🏁 Script executed:
#!/bin/bash # Check if there's a corresponding v3 endpoint that replaces this deprecated one rg -A 2 "router.*prefix=\"/analysis\"|router = APIRouter\(.*analysis.*\)" ibis-server/app/routers/v3/Length of output: 101
Manual Verification Needed: Confirm Presence of v3 Replacement Endpoint
- The deprecated v2 endpoint declaration (
@router.get("/sql", deprecated=True)) inibis-server/app/routers/v2/analysis.pyis correctly marked as deprecated.- However, our search for a corresponding v3 endpoint in
ibis-server/app/routers/v3/produced no results.- Please manually verify whether a replacement endpoint is expected in v3. If it is deliberate to not have a v3 counterpart yet, no fixes are necessary; otherwise, consider adding the appropriate v3 implementation.
ibis-server/app/routers/v3/connector.py (4)
40-58: Well-implemented exception handling with v2 fallback strategyThe implementation properly handles v3 query execution with fallback to v2 when exceptions occur. This approach enables a smooth transition between API versions.
71-86: Consistent error handling pattern for dry-plan endpointThe implementation follows the same error handling pattern as the query endpoint, correctly falling back to v2 when v3 fails.
90-112: Consistent implementation for data source specific dry-planThe implementation properly handles errors and follows the established pattern of falling back to v2 API.
116-142: Proper error handling for validation endpointThe implementation correctly handles validation errors and follows the established pattern of falling back to v2 API.
ibis-server/tests/routers/v3/connector/postgres/test_fallback_v2.py (1)
8-25: Good test setup with explicit expectationsThe manifest is well structured and the comment clearly explains that this is not a valid manifest for v3, which sets expectations for the test cases.
ibis-server/tests/routers/v3/connector/local_file/test_functions.py (4)
10-26: Well-structured manifest for function testingThe manifest definition is clear and focused on the necessary elements for testing functions.
35-41: Proper test fixture setup and cleanupThe fixture correctly sets up the remote function list path before tests and resets it afterward, ensuring test isolation.
43-73: Thorough test for function list with different configurationsThe function list test comprehensively validates the behavior when the remote function list path is set to different values. The assertions are clear and the comment explaining the expected count is helpful.
75-108: Good coverage of scalar and aggregate functionsThe tests for scalar and aggregate functions verify both the response status and the expected data structure, providing good validation of functionality.
ibis-server/app/routers/v2/connector.py (8)
33-35: API endpoint marked as deprecated.The
/{data_source}/queryendpoint is now marked as deprecated. This is part of a systematic approach to indicate that v2 API endpoints should no longer be used in favor of newer alternatives. The functionality remains unchanged.
68-68: API endpoint marked as deprecated.Appropriately marking the validation endpoint as deprecated while maintaining its functionality.
92-94: API endpoint marked as deprecated.The table metadata endpoint is now marked as deprecated. The change is consistent with the other endpoints in this file.
109-113: API endpoint marked as deprecated.The constraints metadata endpoint is now flagged as deprecated while preserving all existing functionality.
128-128: API endpoint marked as deprecated.The database version endpoint is now marked as deprecated, consistent with other v2 endpoints.
133-133: API endpoint marked as deprecated.The dry-plan endpoint is correctly marked as deprecated.
147-147: API endpoint marked as deprecated.Data source specific dry-plan endpoint marked as deprecated, maintaining consistency across the v2 API.
165-165: API endpoint marked as deprecated.The model-substitute endpoint is appropriately marked as deprecated.
Since we upgrade datafusion to 0.46.1 , we can merge the release branch back to the main.
Summary by CodeRabbit
Documentation
API Changes
Upgrade