-
Notifications
You must be signed in to change notification settings - Fork 1
feat(pframes): add keysOnly column option to tableBuilder #1702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| --- | ||
| "@platforma-sdk/workflow-tengo": minor | ||
| --- | ||
|
|
||
| tableBuilder: add `keysOnly` option to `addColumn`/`addColumns`. A keys-only | ||
| column participates in the join (its key space is unioned into the output rows | ||
| and any new axes are surfaced) but its value column is not written to the | ||
| exported file. Useful for expanding the table's key space from a column whose | ||
| values are not needed in the output. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,20 +192,29 @@ tableBuilder := func(format, ...ctxArg) { | |
| * | ||
| * @param query: ColumnQuerySpec (anchored axes reference primary names) | | ||
| * PColumnResult | PlRef | EnrichmentRef | ResolvedEnrichmentRef | ||
| * @param opts: (optional) { header?: string } — column header in exported file | ||
| * @param opts: (optional) { header?: string, keysOnly?: bool } | ||
| * header — column header in exported file. | ||
| * keysOnly — when true, the column participates in the join | ||
| * (its key space is unioned into the output rows | ||
| * and any new axes are surfaced), but its value | ||
| * column is NOT written to the file. `header` is | ||
| * ignored for keys-only columns. | ||
| * @return self (for chaining) | ||
| */ | ||
| addColumn: func(query, ...opts) { | ||
| header := undefined | ||
| keysOnly := false | ||
| if len(opts) > 0 && ll.isMap(opts[0]) { | ||
| header = opts[0].header | ||
| keysOnly = opts[0].keysOnly == true | ||
| } | ||
|
Comment on lines
207
to
210
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the project's general rules, we should implement a typo guard when accepting options maps to validate and reject unknown keys. This prevents misspelled options from being silently ignored. Please add a typo guard to validate the keys of the References
|
||
| if isUnresolved(query) || isEnrichmentRef(query) { | ||
| _hasUnresolved = true | ||
| } | ||
| _columns = append(_columns, { | ||
| query: query, | ||
| header: header, | ||
| keysOnly: keysOnly, | ||
| mode: "single" | ||
| }) | ||
| return self | ||
|
|
@@ -216,15 +225,21 @@ tableBuilder := func(format, ...ctxArg) { | |
| * of pre-resolved columns. Empty match set is allowed (produces no enrichments). | ||
| * | ||
| * @param query: ColumnQuerySpec | (PColumnResult | EnrichmentRef | ResolvedEnrichmentRef)[] | PlRef | ||
| * @param opts: (optional) { headerPrefix?: string, headerSuffix?: string } | ||
| * @param opts: (optional) { headerPrefix?: string, headerSuffix?: string, keysOnly?: bool } | ||
| * keysOnly — when true, every matched column participates in | ||
| * the join (key space unioned, new axes surfaced) but no | ||
| * value columns are written. headerPrefix/headerSuffix are | ||
| * ignored for keys-only columns. | ||
| * @return self (for chaining) | ||
| */ | ||
| addColumns: func(query, ...opts) { | ||
| headerPrefix := undefined | ||
| headerSuffix := undefined | ||
| keysOnly := false | ||
| if len(opts) > 0 && ll.isMap(opts[0]) { | ||
| headerPrefix = opts[0].headerPrefix | ||
| headerSuffix = opts[0].headerSuffix | ||
| keysOnly = opts[0].keysOnly == true | ||
| } | ||
|
Comment on lines
239
to
243
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the project's general rules, we should implement a typo guard when accepting options maps to validate and reject unknown keys. This prevents misspelled options from being silently ignored. Please add a typo guard to validate the keys of the References
|
||
|
|
||
| // Per-element entries. Labeled enrichments override the | ||
|
|
@@ -254,6 +269,7 @@ tableBuilder := func(format, ...ctxArg) { | |
| header: entryHeader, | ||
| headerPrefix: entryPrefix, | ||
| headerSuffix: entrySuffix, | ||
| keysOnly: keysOnly, | ||
| mode: "multi" | ||
| }) | ||
| } | ||
|
|
@@ -267,6 +283,7 @@ tableBuilder := func(format, ...ctxArg) { | |
| query: query, | ||
| headerPrefix: headerPrefix, | ||
| headerSuffix: headerSuffix, | ||
| keysOnly: keysOnly, | ||
| mode: "multi" | ||
| }) | ||
| return self | ||
|
|
@@ -469,7 +486,8 @@ tableBuilder := func(format, ...ctxArg) { | |
| mode: c.mode, | ||
| header: c.header, | ||
| headerPrefix: c.headerPrefix, | ||
| headerSuffix: c.headerSuffix | ||
| headerSuffix: c.headerSuffix, | ||
| keysOnly: c.keysOnly | ||
| } | ||
| if isEnrichmentRef(c.query) { | ||
| resolvedHit := resolveColumn(c.query.hit) | ||
|
|
@@ -560,6 +578,8 @@ tableBuilder := func(format, ...ctxArg) { | |
| if !is_undefined(c.header) { entry.header = c.header } | ||
| if !is_undefined(c.headerPrefix) { entry.headerPrefix = c.headerPrefix } | ||
| if !is_undefined(c.headerSuffix) { entry.headerSuffix = c.headerSuffix } | ||
| // Emit only when set — the template defaults a missing flag to false. | ||
| if c.keysOnly { entry.keysOnly = true } | ||
| columnsMap[string(idx)] = entry | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -408,3 +408,147 @@ tplTest.concurrent( | |
| expect(lines.length).toBe(4); // header + 3 data rows | ||
| }, | ||
| ); | ||
|
|
||
| // keys-only column: an enrichment whose key space is unioned into the output | ||
| // (its keys appear as rows) but whose value column is NOT written to the file. | ||
| // `extra` carries id rows A, C, D — primary carries A, B, C. The full-join | ||
| // surfaces D (a key only the keys-only column knows about) while its value | ||
| // stays out of the table. | ||
| const extraCsv = dedent` | ||
| id,extra | ||
| A,111 | ||
| C,333 | ||
| D,444 | ||
| `; | ||
| const extraSpec = specWithLong("extra", "pl7.app/extra", "Extra"); | ||
|
|
||
| tplTest.concurrent( | ||
| "tableBuilder: keysOnly column expands key space without emitting a value column", | ||
| async ({ helper, expect, driverKit }) => { | ||
| const content = await runEphemeralBuilder(helper, driverKit, { | ||
| format: "tsv", | ||
| imports: { | ||
| main: { csv: primaryCsv, spec: primarySpec }, | ||
| extra: { csv: extraCsv, spec: extraSpec }, | ||
| }, | ||
| primaries: [{ name: "main", sourceImport: "main" }], | ||
| columns: [{ mode: "single", sourceImport: "extra", opts: { keysOnly: true } }], | ||
| }); | ||
| const lines = content.trim().split("\n"); | ||
|
|
||
| // Header has ONLY the primary's axis + value column — the keys-only | ||
| // column's value ("Extra") is absent. This is the core assertion. | ||
| expect(lines[0]).toBe("ID\tScore"); | ||
|
|
||
| // Union of keys {A,B,C} ∪ {A,C,D} = {A,B,C,D}. D comes solely from the | ||
| // keys-only column, proving its key space was unioned into the output. | ||
| // (Assert on the key column rather than full rows: D's Score cell is | ||
| // empty, and content.trim() would strip that trailing tab.) | ||
| const dataLines = lines.slice(1); | ||
| const ids = dataLines.map((l) => l.split("\t")[0]).sort(); | ||
| expect(ids).toEqual(["A", "B", "C", "D"]); | ||
|
|
||
| // D carries no Score value (empty cell) — it contributed only its key. | ||
| const dRow = dataLines.find((l) => l.split("\t")[0] === "D")!; | ||
| expect(dRow.split("\t")[1] ?? "").toBe(""); | ||
| // Existing keys keep their primary value. | ||
| expect(dataLines.find((l) => l.split("\t")[0] === "A")).toBe("A\t10"); | ||
| }, | ||
| ); | ||
|
|
||
| tplTest.concurrent( | ||
| "tableBuilder: same column without keysOnly emits its value column (contrast)", | ||
| async ({ helper, expect, driverKit }) => { | ||
| const content = await runEphemeralBuilder(helper, driverKit, { | ||
| format: "tsv", | ||
| imports: { | ||
| main: { csv: primaryCsv, spec: primarySpec }, | ||
| extra: { csv: extraCsv, spec: extraSpec }, | ||
| }, | ||
| primaries: [{ name: "main", sourceImport: "main" }], | ||
| columns: [{ mode: "single", sourceImport: "extra", opts: { header: "Extra" } }], | ||
| }); | ||
| const lines = content.trim().split("\n"); | ||
|
|
||
| // Without keysOnly the value column IS written, proving the flag is what | ||
| // suppresses it (not some unrelated join behaviour). | ||
| expect(lines[0]).toBe("ID\tScore\tExtra"); | ||
| expect(lines.slice(1).sort()).toEqual(["A\t10\t111", "B\t20\t", "C\t30\t333", "D\t\t444"]); | ||
| }, | ||
| ); | ||
|
|
||
| // keys-only column that introduces a NEW axis. The primary is keyed by `id` | ||
| // only; the keys-only `byRegion` column is keyed by (id, region). The full-join | ||
| // surfaces `region` as a new output column (its keys expand the rows) while the | ||
| // column's own value is NOT written. This guards the "new axes are surfaced" | ||
| // half of the feature — distinct from the shared-axis case above, this test | ||
| // fails if the axis-collection loop skipped keys-only columns. | ||
| const byRegionCsv = dedent` | ||
| id,region,rv | ||
| A,north,1 | ||
| A,south,2 | ||
| B,north,3 | ||
| `; | ||
| const byRegionSpec = { | ||
| axes: [ | ||
| { | ||
| column: "id", | ||
| spec: { | ||
| name: "pl7.app/id", | ||
| type: "String", | ||
| annotations: { [Annotation.Label]: "ID" } satisfies Annotation, | ||
| }, | ||
| }, | ||
| { | ||
| column: "region", | ||
| spec: { | ||
| name: "pl7.app/region", | ||
| type: "String", | ||
| annotations: { [Annotation.Label]: "Region" } satisfies Annotation, | ||
| }, | ||
| }, | ||
| ], | ||
| columns: [ | ||
| { | ||
| column: "rv", | ||
| id: "rv", | ||
| spec: { | ||
| valueType: "Long", | ||
| name: "pl7.app/region-value", | ||
| annotations: { [Annotation.Label]: "RegionValue" } satisfies Annotation, | ||
| }, | ||
| }, | ||
| ], | ||
| storageFormat: "Json", | ||
| partitionKeyLength: 0, | ||
| }; | ||
|
|
||
| tplTest.concurrent( | ||
| "tableBuilder: keysOnly column surfaces a new axis without emitting its value", | ||
| async ({ helper, expect, driverKit }) => { | ||
| const content = await runEphemeralBuilder(helper, driverKit, { | ||
| format: "tsv", | ||
| imports: { | ||
| main: { csv: primaryCsv, spec: primarySpec }, | ||
| byRegion: { csv: byRegionCsv, spec: byRegionSpec }, | ||
| }, | ||
| primaries: [{ name: "main", sourceImport: "main" }], | ||
| columns: [{ mode: "single", sourceImport: "byRegion", opts: { keysOnly: true } }], | ||
| }); | ||
| const lines = content.trim().split("\n"); | ||
|
|
||
| // New axis "Region" is surfaced (between the primary axis and value), but | ||
| // the keys-only column's own value ("RegionValue") is NOT in the header. | ||
| expect(lines[0]).toBe("ID\tRegion\tScore"); | ||
|
|
||
| // Full-join over the (id, region) key space: A expands to north+south | ||
| // (Score broadcast = 10), B to north (20), C has no region row so its | ||
| // Region cell is empty (Score 30). Every row keeps its primary Score. | ||
| expect(lines.slice(1).sort()).toEqual([ | ||
| "A\tnorth\t10", | ||
| "A\tsouth\t10", | ||
| "B\tnorth\t20", | ||
| "C\t\t30", | ||
| ]); | ||
| }, | ||
| ); | ||
|
Comment on lines
408
to
+554
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
All three new tests use Prompt To Fix With AIThis is a comment left during a code review.
Path: tests/workflow-tengo/src/pframes/table-builder.test.ts
Line: 408-554
Comment:
**Missing test coverage for `addColumns` multi-match path with `keysOnly`**
All three new tests use `mode: "single"` (i.e. `addColumn`), so the `addColumns` multi-match code path — where `keysOnly` is threaded through a `multiResult` expansion in `build-table.tpl.tengo` — has no integration test coverage. If something regressed in the `if !is_undefined(c.multiResult)` branch (e.g. the flag silently dropped for one of the sub-entries), the existing tests would not catch it. A fourth test exercising `addColumns` with a multi-match query spec and `keysOnly: true` would close this gap.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Tengo, using a non-boolean value (such as
undefined) as anifcondition will cause a runtime panic (non-boolean condition in if statement). Since primary columns inrawColumnsdo not have thekeysOnlyproperty set,column.keysOnlywill resolve toundefinedand crash the execution.Please change the condition to
column.keysOnly == trueto safely handleundefinedvalues.