chore: Refine the project structure and readme #1133
Conversation
WalkthroughThe changes update several configuration and documentation files. The Dependabot configuration now targets specific directories for Maven and Cargo packages. Multiple GitHub workflow files are adjusted to work within the Changes
Suggested labels
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:
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
🧹 Nitpick comments (7)
README.md (1)
84-90: Verify Module Link for Semantic Core
The newly added "Developer GuideS" section is a valuable addition. However, the link for the semantic core is given as[wren-core](./wren-cores). Please verify that the directory name is correct—if the module is namedwren-core(or perhaps should referencewren-core-legacy), the link may need updating to avoid confusion.wren-core-legacy/README.md (4)
8-10: Typographical Corrections Needed
There are a couple of typos in the text: "recommand" (line 9) should be corrected to "recommend" and "easist" should be "easiest".🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10-10: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
10-12: Fenced Code Block Language Specification
The Docker command code block (lines 10–12) currently lacks a language specifier. It is recommended to add a language (e.g.,bash) for proper syntax highlighting and to adhere to markdown lint guidelines.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10-10: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
15-18: Maven Build Code Block Enhancement
For the Maven build command code block at lines 16–18, please add a language specifier (e.g.,bash) to improve readability and meet markdown standards.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
20-22: Java Command Code Block Language
Similarly, add a language specifier (such asbash) for the fenced code block containing the Java command (lines 20–22) to ensure consistency with markdown best practices.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
ibis-server/README.md (2)
74-76: Grammatical Correction for Tracing Section
In the "Enable Tracing" section (line 75), the phrase "We uses OpenTelemetry" should be corrected to "We use OpenTelemetry" to resolve the grammatical error.🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: The pronoun ‘We’ must be used with a non-third-person form of a verb.
Context: ...ash just run ``` ### Enable Tracing We uses OpenTelemetry as its tracing framework....(NON3PRS_VERB)
11-39: Code Block Language & Formatting Consistency
The provided code blocks within the "Quick Start" and "Running on Docker" sections are generally well formatted with proper language tags (e.g.,yaml,bash). Double-check that all fenced code blocks consistently include a language specifier for enhanced readability and proper syntax highlighting.🧰 Tools
🪛 LanguageTool
[style] ~26-~26: In American English, abbreviations like “etc.” require a period.
Context: ... - ./etc:/usr/src/app/etc ``` Createetcdirectory and create `config.propertie...(ETC_PERIOD)
[grammar] ~26-~26: You used an adverb (‘etc’) instead of an adjective, or a noun (‘directory’) instead of another adjective.
Context: ...d createconfig.propertiesinside the etc directory ```bash mkdir etc cd etc vim config.pro...(A_RB_NN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
wren-core-legacy/.mvn/wrapper/maven-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (8)
.github/dependabot.yml(2 hunks).github/workflows/build-image.yml(3 hunks).github/workflows/maven-tests.yml(1 hunks).github/workflows/stable-release.yml(3 hunks).gitignore(1 hunks)README.md(1 hunks)ibis-server/README.md(1 hunks)wren-core-legacy/README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
wren-core-legacy/README.md
10-10: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
16-16: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (17)
.gitignore (1)
6-6: Enhanced.ideaIgnore Pattern
Using**/.ideanow broadens the ignore rule so that any.ideafolder at any depth is excluded, which aligns well with the reorganized project structure..github/workflows/maven-tests.yml (2)
12-14: Restrict Workflow Trigger with Paths Filter
The addition of thepathsfilter ('wren-core-legacy/**') ensures that this workflow is only triggered by changes within the legacy module, optimizing CI runs.
15-17: Set a Consistent Default Working Directory
Defining a defaultworking-directoryofwren-core-legacyfor run steps centralizes configuration and prevents repetitive path specifications in individual steps..github/workflows/stable-release.yml (3)
96-97: Correct Build Step Directory Context
Addingworking-directory: wren-core-legacyin the Build step ensures that Maven commands run in the expected directory after the project reorganization.
118-120: Ensure Consistency in the Prepare Step
Setting theworking-directorytowren-core-legacyfor the Prepare step aligns it with the new structure, ensuring that file paths (e.g. for version evaluation and artifact movement) are correctly resolved.
127-128: Update Docker Build Context
Changing the Docker build context to./wren-core-legacy/dockerdirects the build process to the appropriate directory that contains legacy Docker configurations..github/workflows/build-image.yml (3)
52-54: Set Working Directory for Build Step
Specifyingworking-directory: wren-core-legacyin the Build step ensures that Maven commands execute in the proper directory, reflecting the new project layout.
73-75: Consistent Working Directory for Prepare Step
By setting theworking-directorytowren-core-legacyin the Prepare step, the build process now uniformly targets the correct directory, reducing potential path issues.
81-83: Refined Docker Build Context
Updating the Docker build context to./wren-core-legacy/dockerensures that the correct set of Docker resources is utilized during the build and push operation.wren-core-legacy/README.md (2)
1-3: Document Title & Introduction Clarity
The title and introductory lines clearly establish the purpose of this legacy module. Verify that the description accurately reflects its usage (e.g. its role for SQL planning and Docker image building).
24-32: IDE Setup & Documentation Clarity
The steps for running the Wren Core in an IDE are generally well-detailed. Confirm that all instructions (e.g., SDK configuration and project import steps) remain up-to-date with the project requirements..github/dependabot.yml (2)
4-4: Maven Directory Update Confirmation
The Maven dependency configuration now points to/wren-core-legacy. Please ensure that all related scripts and references within the repository are updated accordingly.
54-66: New Cargo Dependency Entry Verification
A new Cargo dependency entry has been added for the/wren-core-basedirectory. This configuration is well-formed. Please verify that this directory correctly represents the intended Rust module location and that all dependency updates run as expected.ibis-server/README.md (4)
1-2: Updated Module Title & Description
Changing the title to "Ibis Server Module" and streamlining the introductory paragraph improves clarity regarding the module's role. Confirm that all references (links, descriptions) correctly reflect the current project structure.
8-9: Noting the Migration with Clarity
The notice on line 8 effectively informs users of the migration towren-corewhile recommending the Java engine fromwren-core-legacyfor fallback. Ensure that this guidance aligns with the current operational strategy and that users are aware of the migration implications.
41-72: Local Setup Instructions Verification
The "Running on Local" instructions are comprehensive. Please verify that all tool references (e.g., Python 3.11, casey/just, poetry, Rust, and Cargo) are current and that the steps reflect the latest environment setup practices for the ibis-server module.
74-86: Tracing & Migration Reporting Clarity
The sections on enabling tracing and reporting migration issues are informative. Consider reviewing the wording to ensure clarity—especially how users are instructed to report migration issues and provide log details.🧰 Tools
🪛 LanguageTool
[grammar] ~75-~75: The pronoun ‘We’ must be used with a non-third-person form of a verb.
Context: ...ash just run ``` ### Enable Tracing We uses OpenTelemetry as its tracing framework....(NON3PRS_VERB)
[typographical] ~84-~84: Consider adding a comma here.
Context: ...ight now](./Metrics.md) ## Contributing Please see [CONTRIBUTING.md](docs/CONTRIBUTING...(PLEASE_COMMA)
🪛 markdownlint-cli2 (0.17.2)
77-77: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
ibis-server/README.md (2)
8-9: Migration Note EnhancementThe added note informs users about the migration to [wren-core] while still recommending the use of the legacy Java engine (now in the wren-core-legacy directory) for query fallback. The guidance is useful; however, you may consider rephrasing for an even smoother read. For example:
“Wren Engine is migrating to wren-core. For now, we recommend starting the Java engine to enable the query fallback mechanism.”
75-75: Tracing Framework Description ClarificationThe sentence “We use OpenTelemetry as its tracing framework.” could be clearer. Changing it to “We use OpenTelemetry as our tracing framework.” would improve subject consistency.
wren-core-legacy/README.md (4)
1-3: Legacy Module Documentation and Article UsageLines 1–3 introduce the legacy Wren Core effectively. As a nitpicky suggestion, consider adding the article “the” for clarity in line 3, e.g., “Currently, the Wren engine has been migrated to a new Rust implementation 🚀.”
🧰 Tools
🪛 LanguageTool
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ...wren-engineDocker image. Currently, Wren engine has been migrated to [a new Rust...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
8-12: Docker Run Command Block: Specify Language for ClarityThe command block for running the Wren Core server (lines 10–12) currently lacks a language specifier. Adding one (e.g., using “```bash”) would enhance syntax highlighting and clarity.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
10-10: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
14-18: Maven Build Command Block: Add Language SpecifierFor the Maven build instructions provided in the code block at lines 16–18, please consider specifying the language (such as “```bash”) to improve the readability and consistency of the documentation.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
16-16: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
19-22: Server Startup Command Block: Include Language SpecifierSimilarly, the code block containing the command to start the Wren Core server (lines 20–22) should include a language specifier (e.g., “```bash”) to maintain consistency with the rest of the document.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
20-20: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/README.md(2 hunks)wren-core-legacy/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
wren-core-legacy/README.md
[uncategorized] ~3-~3: You might be missing the article “the” here.
Context: ... wren-engine Docker image. Currently, Wren engine has been migrated to [a new Rust...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 markdownlint-cli2 (0.17.2)
wren-core-legacy/README.md
10-10: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
16-16: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
20-20: 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 (4)
ibis-server/README.md (2)
1-1: Title Update: Clear and Consistent Module NamingThe updated title “# Ibis Server Module” clearly reflects the module's purpose and improves consistency.
2-2: Improved Introduction ClarityThe revised introductory paragraph succinctly explains the module’s role—detailing its FastAPI foundation and the flow from SQL planning to execution via wren-core, sqlglot, and ibis. This provides clear context to readers.
wren-core-legacy/README.md (2)
5-7: Requirements Section Looks GoodThe requirements section clearly specifies the minimum JDK version. No changes suggested.
24-37: IDE Setup Instructions: Clear and InformativeThe guidelines for running Wren Core in an IDE are concise and well organized—from project import instructions in IntelliJ IDEA to SDK configuration and run profile setup. No changes are necessary.
|
The CI failure is the BigQuery secret issue. Let's ignore it. |
Describe
wren-core-legacySummary by CodeRabbit
Chores
Documentation