feat(wren-ui): Add Apache Doris data source support#2150
Open
catpineapple wants to merge 8 commits intoCanner:mainfrom
Open
feat(wren-ui): Add Apache Doris data source support#2150catpineapple wants to merge 8 commits intoCanner:mainfrom
catpineapple wants to merge 8 commits intoCanner:mainfrom
Conversation
wren-ui: - Add Doris to DataSourceName enum, GraphQL schema, and SupportedDataSource - Add DORIS_CONNECTION_INFO interface (host, port, user, password, database) - Add Doris datasource config with sensitiveProps and toIbisConnectionInfo - Add DorisProperties.tsx connection form component and doris.svg icon - Add Doris to WrenEngineDataSourceType and mdlBuilder - Add Doris to frontend DATA_SOURCES enum and DATA_SOURCE_OPTIONS - Add build dependencies (python3, make, g++, cmake) to Dockerfile - Add ignoreBuildErrors for vega-lite@6.x ESM type compatibility - Fix null-safety: add null checks for getLastDeployment() in 5 files (11 locations) wren-launcher: - Add WrenDorisDataSource struct with MapType/GetType/Validate - Add Doris case in ConvertDbtProjectCore wren-engine (submodule update): - Update submodule to feat/add-doris-connector branch
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds Doris as a new data source across launcher, backend, and frontend: DBT conversion and WrenDorisDataSource, new enums/types and repo connection info, MDL mapping and Ibis routing, UI form and utilities, plus runtime guards for missing deployments and a submodule pointer update. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as UI (Doris form)
participant API as GraphQL API
participant Repo as Project Repository
participant MDL as MDL Builder
participant Ibis as Ibis Adaptor
participant Launcher as Launcher (DBT conversion)
participant Engine as Wren Engine
User->>UI: Submit Doris connection & deploy
UI->>API: Save connection / trigger deploy
API->>Repo: Persist/return DORIS_CONNECTION_INFO (encrypted)
API->>Repo: Fetch project & last deployment
Repo-->>API: Return project and last deployment
API->>MDL: Build manifest (map DORIS → engine DORIS)
MDL-->>API: Manifest
API->>Ibis: Route Doris endpoint ('doris')
Ibis->>Launcher: Provide connection info for DBT conversion
Launcher->>Engine: Convert/deploy manifest with Doris datasource
Engine-->>API: Deployment result
API-->>User: Deployment status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
120-134:⚠️ Potential issue | 🟠 MajorSkip SQL validation when
sqlis omitted.
updateSqlPair()accepts optionalsql?: stringbut passes it directly tovalidateSql(sql: string, ctx: IContext), which lacks a null guard. This causes runtime errors on question-only updates. Although TypeScript'sstrictmode is disabled in wren-ui, the logical bug remains.Fix
- private async validateSql(sql: string, ctx: IContext) { + private async validateSql(sql: string | undefined, ctx: IContext) { + if (sql == null) { + return; + } const project = await ctx.projectService.getCurrentProject();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts` around lines 120 - 134, The validateSql method (validateSql) is called with a possibly undefined sql from updateSqlPair, causing runtime errors when sql is omitted; fix by adding a null/empty guard so we skip validation when sql is falsy — either update updateSqlPair to call validateSql only when sql is provided, or change validateSql signature to accept sql?: string and return early if !sql. Keep references to validateSql and updateSqlPair so the caller path is corrected and no preview call is made for missing SQL.
🧹 Nitpick comments (2)
wren-ui/Dockerfile (1)
5-5: Use a minimal apt install here.Line 5 installs
curlwithout--no-install-recommends, so Debian can pull extra packages into every downstream stage. Since this is the sharedbaseimage, that unnecessarily increases the final image size and attack surface. Ifcurlis only needed during build, I'd also keep it out ofrunnerentirely.Diff
-RUN apt-get update && apt-get install -y curl && rm -rf /var/lib/apt/lists/* +RUN apt-get update \ + && apt-get install -y --no-install-recommends curl \ + && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-ui/Dockerfile` at line 5, The RUN line in the Dockerfile installs curl broadly and without --no-install-recommends which inflates the shared base image; update the installation to use apt-get install -y --no-install-recommends curl and keep curl limited to the build stage only (remove it from the shared base/runner stages) — if you need curl only during image build, move the install into the specific build stage (or a separate temporary stage) and ensure you clean apt lists as already done to avoid leaving curl in the final runner image.wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts (1)
102-107: Extract arequireLastDeployment()helper.This lookup/error block is now duplicated in multiple resolvers. Moving it into
deployServicekeeps resolver code thinner and guarantees one error contract.♻️ Suggested extraction
- const lastDeployment = await ctx.deployService.getLastDeployment( - project.id, - ); - if (!lastDeployment) { - throw new Error('No deployment found. Please deploy the model first.'); - } + const lastDeployment = + await ctx.deployService.requireLastDeployment(project.id);As per coding guidelines "Organize wren-ui GraphQL resolvers in
src/apollo/server/resolvers/with corresponding business logic insrc/apollo/server/services/."Also applies to: 122-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts` around lines 102 - 107, Extract the duplicated lookup/error logic into a new deployService method named requireLastDeployment(projectId) that calls the existing getLastDeployment(projectId) and either returns the deployment or throws the same Error('No deployment found. Please deploy the model first.'); then replace occurrences in sqlPairResolver (and the similar block at lines 122-127) to call ctx.deployService.requireLastDeployment(project.id) and use the returned deployment instead of repeating the lookup/throw pattern so resolvers remain thin and error behavior is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wren-launcher/commands/dbt/converter.go`:
- Around line 215-225: The Doris branch currently writes typedDS.Host verbatim
which breaks container mode; modify the WrenDorisDataSource branch that builds
wrenDataSource to call handleLocalhostForContainer(typedDS.Host,
opts.UsedByContainer) (or the existing handleLocalhostForContainer signature
used by Postgres/MSSQL) and use its return value for the "host" property when
opts.UsedByContainer is true so localhost/127.0.0.1 are rewritten to the host
address; update the branch that constructs the map["properties"] for "host"
accordingly and ensure you reference WrenDorisDataSource, wrenDataSource,
handleLocalhostForContainer and opts.UsedByContainer.
In `@wren-ui/src/components/pages/setup/utils.tsx`:
- Around line 120-124: The Doris data source entry currently sets guide to an
empty string which causes ConnectDataSource (which renders <Link
href={current.guide}>) to produce href="" and navigate to the current page;
remove the guide: '' property from the DATA_SOURCES.DORIS object returned/merged
with getDataSourceConfig(DATA_SOURCES.DORIS) so the optional guide field is
omitted (or left undefined) instead of an empty string; update the entry in the
map where DATA_SOURCES.DORIS is defined and ensure no other consumers expect a
non-empty guide.
---
Outside diff comments:
In `@wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts`:
- Around line 120-134: The validateSql method (validateSql) is called with a
possibly undefined sql from updateSqlPair, causing runtime errors when sql is
omitted; fix by adding a null/empty guard so we skip validation when sql is
falsy — either update updateSqlPair to call validateSql only when sql is
provided, or change validateSql signature to accept sql?: string and return
early if !sql. Keep references to validateSql and updateSqlPair so the caller
path is corrected and no preview call is made for missing SQL.
---
Nitpick comments:
In `@wren-ui/Dockerfile`:
- Line 5: The RUN line in the Dockerfile installs curl broadly and without
--no-install-recommends which inflates the shared base image; update the
installation to use apt-get install -y --no-install-recommends curl and keep
curl limited to the build stage only (remove it from the shared base/runner
stages) — if you need curl only during image build, move the install into the
specific build stage (or a separate temporary stage) and ensure you clean apt
lists as already done to avoid leaving curl in the final runner image.
In `@wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts`:
- Around line 102-107: Extract the duplicated lookup/error logic into a new
deployService method named requireLastDeployment(projectId) that calls the
existing getLastDeployment(projectId) and either returns the deployment or
throws the same Error('No deployment found. Please deploy the model first.');
then replace occurrences in sqlPairResolver (and the similar block at lines
122-127) to call ctx.deployService.requireLastDeployment(project.id) and use the
returned deployment instead of repeating the lookup/throw pattern so resolvers
remain thin and error behavior is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 699467cb-9e9a-4462-9367-a29871cf7ba1
⛔ Files ignored due to path filters (1)
wren-ui/public/images/dataSource/doris.svgis excluded by!**/*.svg
📒 Files selected for processing (21)
wren-enginewren-launcher/commands/dbt/converter.gowren-launcher/commands/dbt/data_source.gowren-ui/Dockerfilewren-ui/src/apollo/client/graphql/__types__.tswren-ui/src/apollo/server/adaptors/ibisAdaptor.tswren-ui/src/apollo/server/backgrounds/dashboardCacheBackgroundTracker.tswren-ui/src/apollo/server/dataSource.tswren-ui/src/apollo/server/mdl/mdlBuilder.tswren-ui/src/apollo/server/mdl/type.tswren-ui/src/apollo/server/repositories/projectRepository.tswren-ui/src/apollo/server/resolvers/dashboardResolver.tswren-ui/src/apollo/server/resolvers/modelResolver.tswren-ui/src/apollo/server/resolvers/sqlPairResolver.tswren-ui/src/apollo/server/schema.tswren-ui/src/apollo/server/services/askingService.tswren-ui/src/apollo/server/types/dataSource.tswren-ui/src/components/pages/setup/dataSources/DorisProperties.tsxwren-ui/src/components/pages/setup/utils.tsxwren-ui/src/utils/dataSourceType.tswren-ui/src/utils/enum/dataSources.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
wren-ui/src/components/pages/setup/utils.tsx (1)
120-123:⚠️ Potential issue | 🟠 MajorDoris still ships with an invalid setup-guide link.
Line 121 only spreads
getDataSourceConfig(DATA_SOURCES.DORIS), which does not includeguide, butConnectDataSourcestill renders<Link href={current.guide}>unconditionally. That leaves Doris withhref={undefined}once selected, so the setup screen gets a broken link at best and a render/runtime failure at worst. Please either provide a real Doris guide here or make the consumer skip the link whenguideis absent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wren-ui/src/components/pages/setup/utils.tsx` around lines 120 - 123, The Doris entry is missing a guide URL, causing ConnectDataSource to render <Link href={current.guide}> with an undefined href; fix by adding a valid guide property to the DATA_SOURCES.DORIS config returned here (i.e., include guide: '<valid-doris-setup-url>' alongside the spread of getDataSourceConfig(DATA_SOURCES.DORIS) and disabled: false) so current.guide is defined, or alternatively update the ConnectDataSource component to only render Link when current.guide is truthy (guarding the Link render).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@wren-ui/src/components/pages/setup/utils.tsx`:
- Around line 120-123: The Doris entry is missing a guide URL, causing
ConnectDataSource to render <Link href={current.guide}> with an undefined href;
fix by adding a valid guide property to the DATA_SOURCES.DORIS config returned
here (i.e., include guide: '<valid-doris-setup-url>' alongside the spread of
getDataSourceConfig(DATA_SOURCES.DORIS) and disabled: false) so current.guide is
defined, or alternatively update the ConnectDataSource component to only render
Link when current.guide is truthy (guarding the Link render).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 496c34f3-955e-4bb4-85ac-704756d4d32b
📒 Files selected for processing (3)
wren-launcher/commands/dbt/converter.gowren-ui/src/apollo/server/resolvers/sqlPairResolver.tswren-ui/src/components/pages/setup/utils.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- wren-ui/src/apollo/server/resolvers/sqlPairResolver.ts
Author
|
@andreashimin Hello, could you please review this PR for me? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does:
wren-ui:
wren-launcher:
wren-engine (submodule update):
Summary by CodeRabbit
New Features
Bug Fixes