chore(tool): add manifest extract for query_local_run#1191
chore(tool): add manifest extract for query_local_run#1191douenergy merged 1 commit intoCanner:mainfrom
Conversation
WalkthroughThe script now explicitly imports the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryLocalRunScript
participant wren_core.ManifestExtractor
participant SessionContext
User->>QueryLocalRunScript: Provide SQL query and manifest
QueryLocalRunScript->>wren_core.ManifestExtractor: Initialize with manifest
QueryLocalRunScript->>wren_core.ManifestExtractor: Resolve tables in SQL
wren_core.ManifestExtractor-->>QueryLocalRunScript: Return used tables
QueryLocalRunScript->>wren_core.ManifestExtractor: Extract filtered manifest for used tables
wren_core.ManifestExtractor-->>QueryLocalRunScript: Return filtered manifest
QueryLocalRunScript->>SessionContext: Initialize with filtered manifest
QueryLocalRunScript->>SessionContext: Run query
Suggested labels
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ibis-server/tools/query_local_run.py (1)
25-25: Redundant import and potential namespace confusion
wren_coreis imported in two different styles:from wren_core import SessionContext(line 22) andimport wren_core(line 25). While not wrong, keeping a single, consistent import style helps readability and avoids shadowing surprises ifwren_corelater exposes a top-levelSessionContext.If you only need
SessionContextandManifestExtractoryou can shorten the import list and drop the second statement:-from wren_core import SessionContext -import wren_core +from wren_core import SessionContext, ManifestExtractor, to_json_base64…and then use
ManifestExtractor/to_json_base64directly.
Otherwise, consider aliasing (import wren_core as wc) so it is obvious which names come from the top-level package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ibis-server/tools/query_local_run.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
ibis-server/tools/query_local_run.py (2)
wren-core-py/src/extractor.rs (2)
extractor(221-223)extract_by(40-42)wren-core-base/manifest-macro/src/lib.rs (1)
manifest(26-56)
🪛 Ruff (0.8.2)
ibis-server/tools/query_local_run.py
64-64: print found
(T201)
69-69: print found
(T201)
70-70: print found
(T201)
73-73: print found
(T201)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: validate-pull-request-title
- GitHub Check: ci
🔇 Additional comments (1)
ibis-server/tools/query_local_run.py (1)
76-76: Verify impact of removing explicit source dialectChanging
read="trino"→read=Noneletssqlglotauto-detect the dialect.
Auto-detection works most of the time but can mis-classify queries with vendor-specific
functions or identifiers (e.g.,IFnull, back-tick quoting). A mis-detected dialect will
lead to silent rewrites and potentially invalid SQL on the target engine.Please test against a representative set of queries for each supported source to ensure
no regressions, or gate this change behind a switch.
|
This is very useful. 👍 |
To align the behavior of the real query scenario. I added the manifest extractor to the
query_local_run.pyscript.Summary by CodeRabbit