Repository navigation
Add setColumnJoin and a per-column join option to tableBuilder - #1897
julenmendieta wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: ea179e1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * Sets how columns added with addColumn / addColumns are joined onto the | ||
| * trunk (the primaries, after their filters and setJoinMode). A column's own | ||
| * `join` option overrides it. | ||
| * "full" (default) — keep the union; columns can bring back keys a filter removed | ||
| * "left" — keep exactly the trunk's keys; columns only fill values. Use it | ||
| * with filtered primaries so added columns respect the filter. | ||
| * "inner" — keep only keys every such column also has |
There was a problem hiding this comment.
setColumnJoin describes each mode alone, but mixed modes always run in the order inner, left, then full, regardless of call order. If the primary has A and both a left-joined column and a full-joined column have B, the result includes B with an empty left-column cell. Document this order and explain that full-added rows bypass the other columns' joins.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/workflow-tengo/src/pframes/table-builder.lib.tengo
Line: 325-331
Comment:
**Mixed join order is undocumented**
`setColumnJoin` describes each mode alone, but mixed modes always run in the order `inner`, `left`, then `full`, regardless of call order. If the primary has A and both a left-joined column and a full-joined column have B, the result includes B with an empty left-column cell. Document this order and explain that full-added rows bypass the other columns' joins.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
| tplTest.concurrent( | ||
| "tableBuilder: a column's own join overrides setColumnJoin", | ||
| async ({ helper, expect, driverKit }) => { | ||
| const content = await runEphemeralBuilder(helper, driverKit, { | ||
| ...filteredPrimaryWithColumn, | ||
| columns: [{ mode: "single", sourceImport: "extra", opts: { join: "full" } }], | ||
| columnJoin: "left", | ||
| }); | ||
| const lines = content.trim().split("\n"); | ||
|
|
||
| expect(lines.slice(1).sort()).toEqual(["A\t10\t100", "B\t\t200", "C\t30\t300"]); |
There was a problem hiding this comment.
The new tests do not combine a full-joined column with another left- or inner-joined column. The override test has only one added column, so it cannot catch changes to the deliberate join order. Add a mixed-mode test where a full join restores a removed key and assert that the left and inner columns remain empty on that row.
Prompt To Fix With AI
This is a comment left during a code review.
Path: tests/workflow-tengo/src/pframes/table-builder.test.ts
Line: 271-281
Comment:
**Mixed full joins lack coverage**
The new tests do not combine a full-joined column with another left- or inner-joined column. The override test has only one added column, so it cannot catch changes to the deliberate join order. Add a mixed-mode test where a full join restores a removed key and assert that the left and inner columns remain empty on that row.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1897 +/- ##
=======================================
Coverage 57.31% 57.31%
=======================================
Files 446 446
Lines 23309 23309
Branches 5254 5254
=======================================
Hits 13359 13359
Misses 8406 8406
Partials 1544 1544 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
The PR appears safe to merge, with non-blocking improvements needed for mixed-mode documentation and tests.
Fix with agent prompt
Summary
Adds
setColumnJoinand a per-calljoinoption toaddColumnandaddColumns. The default stays "full"; "left" keeps filtered primary keys, and "inner" requires matching keys.Important touched terms:
TableBuilder: The builder that collects primary and added columns for table export. It gainssetColumnJoinand per-call join overrides.PEntry: A table-query entry carrying columns and a query. The template now nests these entries throughpt.p.inner,pt.p.outer, andpt.p.full.SpecQueryJoinEntry: A query wrapped for use inside a join. Linker-based columns continue to use this shape, now placed in the selected join group.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR P[Primaries] --> F[Apply primary filters] F --> T[Combine primaries] T --> I[Join inner columns] I --> L[Join left columns] L --> U[Join full columns] U --> E[Export table]Reviews (1) · Last reviewed commit: "Add setColumnJoin and a per-column join ..." · Reviewed by Greptile