Data dependency#933
Open
NQPhuc wants to merge 9 commits into
Open
Conversation
Benchmark Resultdbml-parse
|
Coverage ReportCommit: d522723 Overall Coverage
Package Coverage
|
| File | Lines | Statements | Functions | Branches |
|---|---|---|---|---|
src/connectors/bigquery/index.ts |
0.00% | 0.00% | 0.00% | 0.00% |
src/utils/credential-loader.ts |
0.00% | 0.00% | 0.00% | 0.00% |
src/utils/helpers.ts |
0.00% | 0.00% | 0.00% | 0.00% |
src/connectors/snowflake/index.ts |
10.56% | 10.31% | 0.00% | 0.00% |
src/utils/parseSchema.ts |
46.15% | 42.85% | 28.57% | 27.27% |
src/connectors/connector.ts |
66.66% | 66.66% | 100.00% | 57.14% |
src/connectors/oracle/tables.ts |
71.25% | 67.39% | 100.00% | 56.96% |
src/connectors/oracle/index.ts |
80.00% | 80.76% | 100.00% | 25.00% |
src/connectors/oracle/utils.ts |
85.71% | 85.71% | 100.00% | 71.42% |
dbml-parse
83 file(s) below 80% coverage
| File | Lines | Statements | Functions | Branches |
|---|---|---|---|---|
src/compiler/queries/container/scope.ts |
0.00% | 0.00% | 0.00% | 0.00% |
src/services/diagnostics/provider.ts |
0.00% | 0.00% | 0.00% | 0.00% |
src/compiler/queries/pipeline/interpret.ts |
11.76% | 13.51% | 66.66% | 41.66% |
src/services/suggestions/crossFile.ts |
19.35% | 19.44% | 33.33% | 10.00% |
src/core/local_modules/enum/index.ts |
48.00% | 46.29% | 57.14% | 52.63% |
src/core/global_modules/project/interpret.ts |
52.50% | 46.66% | 83.33% | 23.07% |
src/core/local_modules/dep/index.ts |
53.12% | 52.94% | 100.00% | 46.42% |
src/compiler/projectLayout/layout.ts |
54.16% | 53.84% | 75.00% | 52.63% |
src/compiler/queries/legacy/parse.ts |
61.53% | 53.33% | 66.66% | 25.00% |
src/core/local_modules/ref/index.ts |
62.50% | 61.76% | 100.00% | 57.14% |
src/core/types/report.ts |
62.50% | 55.55% | 75.00% | 57.14% |
src/core/types/symbol/symbols.ts |
62.54% | 55.20% | 59.50% | 45.71% |
src/core/global_modules/records/utils/data/values.ts |
63.30% | 55.71% | 72.72% | 49.28% |
src/core/local_modules/tableGroup/validate.ts |
64.55% | 64.44% | 71.42% | 58.00% |
src/compiler/queries/legacy/token.ts |
66.66% | 66.66% | 66.66% | 100.00% |
src/core/global_modules/enum/bind.ts |
66.66% | 68.75% | 83.33% | 50.00% |
src/core/local_modules/program/index.ts |
66.66% | 69.23% | 75.00% | 62.50% |
src/core/local_modules/dep/validate.ts |
68.75% | 63.63% | 84.61% | 50.00% |
src/core/global_modules/note/bind.ts |
69.23% | 71.42% | 83.33% | 50.00% |
src/core/local_modules/checks/index.ts |
69.23% | 75.00% | 100.00% | 64.28% |
src/core/local_modules/project/index.ts |
73.07% | 73.07% | 100.00% | 68.18% |
src/compiler/queries/utils.ts |
73.10% | 73.38% | 90.90% | 51.11% |
src/core/local_modules/tablePartial/validate.ts |
73.12% | 71.83% | 79.22% | 57.67% |
src/core/global_modules/project/bind.ts |
73.33% | 75.00% | 100.00% | 50.00% |
src/core/local_modules/indexes/index.ts |
73.68% | 69.35% | 63.63% | 64.28% |
src/core/local_modules/use/index.ts |
73.91% | 75.00% | 75.00% | 81.25% |
src/core/local_modules/indexes/validate.ts |
74.28% | 74.66% | 90.90% | 56.00% |
src/core/local_modules/note/index.ts |
75.00% | 75.86% | 75.00% | 72.72% |
src/services/suggestions/utils/index.ts |
75.00% | 75.00% | 100.00% | 60.71% |
src/core/local_modules/records/index.ts |
75.60% | 76.19% | 100.00% | 68.08% |
src/core/local_modules/project/validate.ts |
75.86% | 75.86% | 100.00% | 56.25% |
src/core/global_modules/indexes/bind.ts |
76.31% | 74.35% | 90.90% | 68.96% |
src/core/local_modules/custom/index.ts |
76.47% | 76.19% | 80.00% | 75.00% |
src/core/global_modules/records/utils/data/sqlTypes.ts |
76.59% | 80.64% | 82.35% | 72.22% |
src/core/global_modules/utils.ts |
76.92% | 73.80% | 62.50% | 62.50% |
src/core/global_modules/tablePartial/interpret.ts |
78.12% | 70.90% | 78.94% | 53.73% |
src/core/types/filepath.ts |
79.48% | 78.57% | 77.77% | 70.37% |
src/core/global_modules/program/utils.ts |
80.00% | 80.00% | 100.00% | 70.00% |
src/core/global_modules/tableGroup/bind.ts |
80.00% | 80.00% | 100.00% | 57.14% |
src/core/local_modules/records/validate.ts |
80.28% | 80.55% | 93.75% | 72.30% |
src/core/global_modules/indexes/interpret.ts |
81.39% | 72.00% | 100.00% | 56.66% |
src/core/local_modules/tablePartial/index.ts |
81.48% | 81.48% | 100.00% | 77.27% |
src/core/local_modules/diagramView/validate.ts |
81.57% | 78.04% | 84.21% | 72.72% |
src/core/local_modules/checks/validate.ts |
81.63% | 82.69% | 93.75% | 70.00% |
src/core/local_modules/note/validate.ts |
82.00% | 81.81% | 88.88% | 76.31% |
__tests__/utils/compiler.ts |
82.14% | 80.10% | 100.00% | 57.26% |
src/core/global_modules/records/bind.ts |
82.92% | 83.72% | 93.75% | 67.30% |
src/compiler/queries/container/token.ts |
83.33% | 85.71% | 100.00% | 75.00% |
src/core/parser/parser.ts |
83.71% | 83.94% | 98.38% | 75.51% |
src/core/global_modules/dep/interpret.ts |
83.75% | 78.88% | 83.33% | 62.90% |
src/core/global_modules/ref/bind.ts |
83.87% | 83.87% | 90.00% | 75.00% |
src/core/global_modules/records/utils/constraints/pk.ts |
84.61% | 81.42% | 92.59% | 56.52% |
src/core/local_modules/table/validate.ts |
85.10% | 85.62% | 90.47% | 72.63% |
src/core/local_modules/enum/validate.ts |
85.48% | 81.81% | 81.25% | 76.31% |
__tests__/utils/mocks.ts |
85.71% | 86.20% | 75.00% | 100.00% |
src/core/global_modules/use/index.ts |
86.59% | 83.20% | 100.00% | 78.22% |
src/services/suggestions/utils/useMerger.ts |
86.95% | 81.13% | 100.00% | 46.34% |
src/core/global_modules/tablePartial/bind.ts |
87.03% | 87.27% | 100.00% | 71.42% |
src/core/global_modules/tableGroup/interpret.ts |
88.37% | 88.37% | 100.00% | 77.27% |
src/services/suggestions/recordRowSnippet.ts |
88.46% | 87.09% | 100.00% | 73.80% |
src/compiler/queries/nodeAtPosition.ts |
88.88% | 90.00% | 100.00% | 75.00% |
src/core/global_modules/diagramView/index.ts |
89.16% | 86.23% | 83.33% | 78.18% |
src/services/suggestions/provider.ts |
89.37% | 85.26% | 98.41% | 77.07% |
src/compiler/index.ts |
90.76% | 90.83% | 75.00% | 78.84% |
src/core/global_modules/records/index.ts |
90.90% | 87.15% | 100.00% | 79.16% |
src/core/types/symbol/metadata.ts |
91.81% | 80.00% | 98.43% | 65.46% |
src/core/global_modules/table/interpret.ts |
91.89% | 83.52% | 95.83% | 66.66% |
src/core/global_modules/schema/index.ts |
92.26% | 82.46% | 91.66% | 70.07% |
src/core/utils/interpret.ts |
93.33% | 90.00% | 100.00% | 73.23% |
src/core/global_modules/diagramView/interpret.ts |
94.59% | 90.40% | 92.85% | 75.75% |
src/services/suggestions/use.ts |
94.68% | 87.61% | 100.00% | 77.45% |
src/core/global_modules/dep/bind.ts |
95.00% | 84.00% | 100.00% | 58.33% |
src/core/global_modules/note/interpret.ts |
95.65% | 95.65% | 100.00% | 75.00% |
src/core/global_modules/records/interpret.ts |
95.97% | 92.59% | 100.00% | 79.54% |
__tests__/examples/interpreter/multifile/utils.ts |
96.00% | 89.65% | 100.00% | 62.50% |
src/compiler/queries/canonicalName.ts |
96.96% | 92.10% | 100.00% | 76.66% |
src/core/global_modules/program/interpret.ts |
97.35% | 92.16% | 100.00% | 70.50% |
src/core/types/errors.ts |
98.50% | 98.50% | 66.66% | 100.00% |
src/compiler/queries/transform/renameTable.ts |
99.05% | 90.47% | 100.00% | 78.82% |
src/core/global_modules/checks/interpret.ts |
100.00% | 88.88% | 100.00% | 60.00% |
src/core/global_modules/program/index.ts |
100.00% | 85.71% | 100.00% | 75.00% |
src/core/global_modules/records/utils/constraints/unique.ts |
100.00% | 95.00% | 100.00% | 62.50% |
src/services/definition/provider.ts |
100.00% | 95.23% | 100.00% | 71.42% |
huydo862003
reviewed
Jun 25, 2026
Comment on lines
365
to
+595
| @@ -575,6 +586,13 @@ export function validateFieldSetting (parts: ExpressionNode[]): Report<Settings> | |||
| } | |||
| }); | |||
| break; | |||
| case SettingName.Dep: | |||
| attrs.forEach((attr) => { | |||
| if (!isUnaryRelationship(attr.value)) { | |||
| errors.push(new CompileError(CompileErrorCode.INVALID_COLUMN_SETTING_VALUE, '\'dep\' must be a valid unary relationship', attr.value || attr.name!)); | |||
| } | |||
| }); | |||
| break; | |||
Contributor
There was a problem hiding this comment.
I suggest these two validations can use a reusable isUnaryDependency function like isUnaryRelationship.
Currnently, this doesn't raise any error, as you only validate the dep op, not the dep operands yet, for this, you can take isUnaryRelationship for reference. 👀
huydo862003
reviewed
Jun 25, 2026
Comment on lines
+124
to
+127
| export function validateDepSettings (settings: ListExpressionNode): Report<Settings> { | ||
| const aggReport = aggregateSettingList(settings); | ||
| return new Report(aggReport.getValue(), aggReport.getErrors()); | ||
| } |
Contributor
There was a problem hiding this comment.
Does dep allow any setting list? if not, should disallow, see CustomValidator.validateSettingList for more details.
Then this function should also be called inside DepValidator.validate() too, else the validate flow wouldn't catch it 👀
huydo862003
reviewed
Jun 25, 2026
| if (!(this.declarationNode.parent instanceof ElementDeclarationNode && this.declarationNode.parent.isKind(ElementKind.Project, ElementKind.Dep))) { | ||
| return [ | ||
| new CompileError(CompileErrorCode.INVALID_CUSTOM_CONTEXT, 'A Custom element can only appear in a Project', this.declarationNode), | ||
| new CompileError(CompileErrorCode.INVALID_CUSTOM_CONTEXT, 'A Custom element can only appear in a Project or a Dep', this.declarationNode), |
Contributor
There was a problem hiding this comment.
Which custom element is allowed in Dep :V?
A custom element accept any name, such as:
Dep {
a: "..." // A custom
b: 3 // A custom
Table.col -> Table.col // Not a custom
}
Contributor
|
Pls help run lint:fix nhe :v |
huydo862003
reviewed
Jun 26, 2026
| return inlineRef; | ||
| }); | ||
|
|
||
| column.inline_deps = []; |
Contributor
There was a problem hiding this comment.
Can we have a comment like: columns in partial table do not support dep, so hardcode to []
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+196
to
+205
| if (isInsideSettingValue(node, SettingName.Dep)) { | ||
| const enclosingDecl = node.parent; | ||
| if (enclosingDecl instanceof ElementDeclarationNode && enclosingDecl.isKind(ElementKind.Table)) { | ||
| const programNode = compiler.parseFile(node.filepath).getValue().ast; | ||
| const globalSymbol = compiler.nodeSymbol(programNode).getValue(); | ||
| if (globalSymbol === UNHANDLED) return Report.create(undefined); | ||
| return nodeRefereeOfInlineDep(compiler, globalSymbol, node); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Why do we check for isInsideSettingValue(Dep) in 2 places? 👀
According to the current pattern, we should add a check like below, not here?
huydo862003
reviewed
Jun 26, 2026
| return nodeRefereeOfInlineDep(compiler, globalSymbol, node); | ||
| } | ||
|
|
||
| // Case 3: Column's default value being an enum value |
Contributor
There was a problem hiding this comment.
Add a case comment here? 👀
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+417
to
+422
| const enclosingFa = node.parentOfKind(FunctionApplicationNode); | ||
| if (enclosingFa) { | ||
| return nodeRefereeOfInlineRef(compiler, globalSymbol, node); | ||
| } | ||
|
|
||
| if (!isExpressionAVariableNode(node)) return new Report(undefined); |
Contributor
There was a problem hiding this comment.
Line 418 should always hold true right? is the code following (421 onwards) deadcode :?
Also, may be consider extracting the shared logic of inline ref that resolve the ref/dep operand then make nodeRefereeOfInlineDep and nodeRefereeOfInlineRef reuse this :v I don't think nodeRefereeOfInlineDep calling nodeRefereeOfINlineRef makes sense.
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+331
to
+332
| const direction = prefix.op?.value as '->' | '<-' | undefined; | ||
| if (direction !== '->' && direction !== '<-') return []; |
Contributor
There was a problem hiding this comment.
:v may be consider make -> and <- check a reusable func. We already have isRelationshipOp. maybe add isDependencyOp
huydo862003
reviewed
Jun 26, 2026
| export type RelationCardinality = '1' | '*'; | ||
|
|
||
| export interface Dep { | ||
| schemaName: string | null; |
Contributor
There was a problem hiding this comment.
Although Ref also has a schemaName field... I don't know if they can have schema name in reality? 👀
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+200
to
+203
| note?: { | ||
| value: string; | ||
| token: TokenPosition; | ||
| }; |
Contributor
There was a problem hiding this comment.
I'm wondering:
Dep [note: 'asdasd'] {
table.col -> table.col [note: 'asdasd'] // can we have note here? If yes, where is it stored in this?
}
Contributor
There was a problem hiding this comment.
In the proposal, I see this case:
Dep: raw_users.email -> stg_users.email [note: 'Lowercase + validate email']
In this case, the note is treated in DBML as on the field, not on the Dep element :v
huydo862003
reviewed
Jun 26, 2026
| } | ||
| } | ||
|
|
||
| export class DepMetadata extends NodeMetadata { |
Contributor
There was a problem hiding this comment.
There's a problem with this:
// utils.dbml
Table T {
id int
}
Table R {
id int
}
Table E {
id int
}
Dep {
T.id -> R.id
R.id -> E.id
}
// main.dbml
use {
table T
table R
} from './utils'
/* T.id -> R.id should be in scope, but because E is not in scope, the WHOLE dep is dropped, so nothing is in scope */
My current proposal:
- Add a new
DepEdgeMetadata.nodeMetadataof aDepfield should return aDepEdgeMetadata. ownersofDepMetadatashould decide whether theDepEdgeMetadatashould be included in the output file, not the whole DepMetadata.- DepMetadata should scan its body and return a DepEdgeMetadata for each.
- When interpret DepMetadata, filter out the DepEdgeMetadata that truly belongs to this file.
huydo862003
reviewed
Jun 26, 2026
huydo862003
left a comment
huydo862003
left a comment
Contributor
There was a problem hiding this comment.
Please implement the language services for dep luôn nha :v can reference ref
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+480
to
+481
| enumIds, tableIds, tableGroupIds, refIds, depIds, | ||
| } = model.schemas[schemaId] as any; |
Contributor
There was a problem hiding this comment.
why do we need as any here :v?
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+352
to
+356
| static formatDepCustomValue (value: unknown): string { | ||
| if (typeof value === 'string') return `'${value.replaceAll('\\', '\\\\').replaceAll('\'', '\\\'')}'`; | ||
| if (value === null || value === undefined) return 'null'; | ||
| return String(value); | ||
| } |
Contributor
There was a problem hiding this comment.
I think you can check formatRecordValue? :v or some function like escapeString in dbml-parse
huydo862003
reviewed
Jun 26, 2026
| } | ||
|
|
||
| let str = 'Dep {\n'; | ||
| edges.forEach((edge: any) => { |
Contributor
There was a problem hiding this comment.
:v can we avoid any here?
huydo862003
reviewed
Jun 26, 2026
| static exportDeps (depIds: number[], model: NormalizedModel): string { | ||
| const strArr = depIds.map((depId) => { | ||
| const dep = (model as any).deps[depId]; | ||
| const edges = dep.edgeIds.map((eid: number) => (model as any).depEdges[eid]); |
Contributor
There was a problem hiding this comment.
Can we avoid any here :v
huydo862003
reviewed
Jun 26, 2026
Comment on lines
+12
to
+37
| function extractSourceTablesFromCtx (ctx, refClassNames) { | ||
| if (!ctx) return []; | ||
| const seen = new Map(); | ||
| const stack = [ctx]; | ||
| while (stack.length > 0) { | ||
| const node = stack.pop(); | ||
| if (!node) continue; | ||
| const className = node.constructor && node.constructor.name; | ||
| if (className && refClassNames.includes(className)) { | ||
| const text = (node.getText && node.getText()) || ''; | ||
| const cleaned = text.replace(/`/g, '').replace(/"/g, '').split(/\s+/)[0]; | ||
| const parts = cleaned.split('.'); | ||
| const tableName = parts[parts.length - 1]; | ||
| const schemaName = parts.length > 1 ? parts[parts.length - 2] : undefined; | ||
| const key = `${schemaName || 'public'}.${tableName}`; | ||
| if (tableName && !seen.has(key)) { | ||
| seen.set(key, { name: tableName, schemaName }); | ||
| } | ||
| } | ||
| const childCount = typeof node.getChildCount === 'function' ? node.getChildCount() : 0; | ||
| for (let i = 0; i < childCount; i++) { | ||
| stack.push(node.getChild(i)); | ||
| } | ||
| } | ||
| return Array.from(seen.values()); | ||
| } |
Contributor
There was a problem hiding this comment.
These extractSourceTablesFromCtx functions are weird:
- The logic is pretty dense, hard to follow.
- Heavy regex manipulation, which is fragile.
- Highly rely on adhoc-check/runtime reflection:
typeof node.getChildCountnode.constructor.namestored in an array...
Can you think of a better way to do this?
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
2 participants
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.
Summary
Issue
(issue link here)
Lasting Changes (Technical)
(please list down: code changes/things that have wide-effect; new libraries/functions added that can be used by others; examples below)
class EmailValidatorto validate email address' validity)Tenant#is_trial?check)Checklist
Please check directly on the box once each of these are done