From db1aca7221b53e2fbc145c45200430e76ac0d01b Mon Sep 17 00:00:00 2001 From: Gaurav Bajpai Date: Fri, 24 Apr 2026 10:19:36 +0530 Subject: [PATCH] fix: address PRizm review findings from internal port PR @W-22196528@ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses 4 critical findings surfaced during PRizm code review of the internal plugin port of this skill (internal PR #19). Applying the same fixes here keeps the external canonical source and the internal port in sync. 1. Test cleanup discipline: switch `test_invalid_json_raises` from `try/finally` to `self.addCleanup(p.unlink, missing_ok=True)` — the unittest-idiomatic way to guarantee temp-file cleanup regardless of how the test exits. 2. `floor()` description in bdt-function-catalog.md: the old row was self-contradictory ("toward zero" AND "toward next integer up" in the same cell). Replace with a single coherent definition: rounds toward negative infinity; for negatives rounds away from zero (e.g., `floor(-2.3) = -3`). 3. Split-node documentation in bdt-node-catalog.md: the old doc claimed `split` routes rows into downstream branches via `branches[]` with per-branch predicates. That is not the canonical schema. Per `SplitParametersInputRepresentation` in core-262-public, `split` is a string-splitting operation: one `sourceField` + `delimiter` → N `targetFields` (one row in, one row out; columns added). Rewrote the section with the correct parameters, lineage effect, gotchas, and a canonical example. Row-routing belongs in `filter` nodes. 4. Sample `assets/sample_bdts/append_and_split.json`: the old sample used the invented `branches[]` shape AND routed the same split into two downstream outputs that each expected different rows — which is not how `split` works. Rewrote the sample so: - `appendV2` unions two order sources (unchanged intent). - `split` uses canonical `{sourceField, delimiter, targetFields}` splitting `CustomerFullName__c` into first + last name columns. - One downstream output consumes the new columns (removes the fake two-branch fan-out). Tests: 92/92 passing. Sample parses and runs through `bdt_analyze.py summary` cleanly (5 nodes: 2 load + 1 appendV2 + 1 split + 1 outputD360). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../assets/sample_bdts/append_and_split.json | 69 ++++++++----------- .../references/bdt-function-catalog.md | 2 +- .../references/bdt-node-catalog.md | 33 +++++++-- .../tests/test_bdt_analyze.py | 10 ++- 4 files changed, 60 insertions(+), 54 deletions(-) diff --git a/skills/explaining-batch-data-transform/assets/sample_bdts/append_and_split.json b/skills/explaining-batch-data-transform/assets/sample_bdts/append_and_split.json index 94eba99..3e79fd7 100644 --- a/skills/explaining-batch-data-transform/assets/sample_bdts/append_and_split.json +++ b/skills/explaining-batch-data-transform/assets/sample_bdts/append_and_split.json @@ -6,7 +6,7 @@ "sources": [], "parameters": { "dataset": {"name": "WebOrders__dlo", "type": "dataLakeObject"}, - "fields": ["OrderId__c", "Amount__c", "Channel__c"], + "fields": ["OrderId__c", "Amount__c", "Channel__c", "CustomerFullName__c"], "sampleDetails": {"type": "TopN", "sortBy": []} } }, @@ -15,7 +15,7 @@ "sources": [], "parameters": { "dataset": {"name": "StoreOrders__dlo", "type": "dataLakeObject"}, - "fields": ["OrderId__c", "Amount__c", "Channel__c"], + "fields": ["OrderId__c", "Amount__c", "Channel__c", "CustomerFullName__c"], "sampleDetails": {"type": "TopN", "sortBy": []} } }, @@ -25,66 +25,55 @@ "parameters": { "allowImplicitDisjointSchema": false, "fieldMappings": [ - {"targetField": "OrderId__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "OrderId__c"}, {"node": "LOAD_STORE_ORDERS", "field": "OrderId__c"}]}, - {"targetField": "Amount__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "Amount__c"}, {"node": "LOAD_STORE_ORDERS", "field": "Amount__c"}]}, - {"targetField": "Channel__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "Channel__c"}, {"node": "LOAD_STORE_ORDERS", "field": "Channel__c"}]} + {"targetField": "OrderId__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "OrderId__c"}, {"node": "LOAD_STORE_ORDERS", "field": "OrderId__c"}]}, + {"targetField": "Amount__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "Amount__c"}, {"node": "LOAD_STORE_ORDERS", "field": "Amount__c"}]}, + {"targetField": "Channel__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "Channel__c"}, {"node": "LOAD_STORE_ORDERS", "field": "Channel__c"}]}, + {"targetField": "CustomerFullName__c", "sources": [{"node": "LOAD_WEB_ORDERS", "field": "CustomerFullName__c"}, {"node": "LOAD_STORE_ORDERS", "field": "CustomerFullName__c"}]} ] } }, - "SPLIT_BY_AMOUNT": { + "SPLIT_CUSTOMER_NAME": { "action": "split", "sources": ["APPEND_ALL_ORDERS"], "parameters": { - "branches": [ - {"name": "high_value", "expression": "Amount__c >= 1000"}, - {"name": "low_value", "expression": "Amount__c < 1000"} + "sourceField": "CustomerFullName__c", + "delimiter": " ", + "targetFields": [ + {"name": "CustomerFirstName__c", "label": "Customer First Name"}, + {"name": "CustomerLastName__c", "label": "Customer Last Name"} ] } }, - "OUTPUT_HIGH": { + "OUTPUT_ORDERS": { "action": "outputD360", - "sources": ["SPLIT_BY_AMOUNT"], + "sources": ["SPLIT_CUSTOMER_NAME"], "parameters": { - "name": "HighValueOrders__dlm", + "name": "OrdersWithCustomerName__dlm", "type": "dataModelObject", "writeMode": "OVERWRITE", "fieldsMappings": [ - {"sourceField": "OrderId__c", "targetField": "OrderId__c"}, - {"sourceField": "Amount__c", "targetField": "Amount__c"}, - {"sourceField": "Channel__c", "targetField": "Channel__c"} - ] - } - }, - "OUTPUT_LOW": { - "action": "outputD360", - "sources": ["SPLIT_BY_AMOUNT"], - "parameters": { - "name": "LowValueOrders__dlm", - "type": "dataModelObject", - "writeMode": "OVERWRITE", - "fieldsMappings": [ - {"sourceField": "OrderId__c", "targetField": "OrderId__c"}, - {"sourceField": "Amount__c", "targetField": "Amount__c"}, - {"sourceField": "Channel__c", "targetField": "Channel__c"} + {"sourceField": "OrderId__c", "targetField": "OrderId__c"}, + {"sourceField": "Amount__c", "targetField": "Amount__c"}, + {"sourceField": "Channel__c", "targetField": "Channel__c"}, + {"sourceField": "CustomerFirstName__c", "targetField": "FirstName__c"}, + {"sourceField": "CustomerLastName__c", "targetField": "LastName__c"} ] } } }, "ui": { "nodes": { - "LOAD_WEB_ORDERS": {"label": "Web Orders", "type": "LOAD_DATASET", "top": 100, "left": 100}, - "LOAD_STORE_ORDERS": {"label": "Store Orders", "type": "LOAD_DATASET", "top": 260, "left": 100}, - "APPEND_ALL_ORDERS": {"label": "Union", "type": "APPEND", "top": 180, "left": 260}, - "SPLIT_BY_AMOUNT": {"label": "Split by $", "type": "SPLIT", "top": 180, "left": 420}, - "OUTPUT_HIGH": {"label": "High value", "type": "OUTPUT", "top": 100, "left": 580}, - "OUTPUT_LOW": {"label": "Low value", "type": "OUTPUT", "top": 260, "left": 580} + "LOAD_WEB_ORDERS": {"label": "Web Orders", "type": "LOAD_DATASET", "top": 100, "left": 100}, + "LOAD_STORE_ORDERS": {"label": "Store Orders", "type": "LOAD_DATASET", "top": 260, "left": 100}, + "APPEND_ALL_ORDERS": {"label": "Union", "type": "APPEND", "top": 180, "left": 260}, + "SPLIT_CUSTOMER_NAME": {"label": "Split full name", "type": "SPLIT", "top": 180, "left": 420}, + "OUTPUT_ORDERS": {"label": "Orders with name", "type": "OUTPUT", "top": 180, "left": 580} }, "connectors": [ - {"source": "LOAD_WEB_ORDERS", "target": "APPEND_ALL_ORDERS"}, - {"source": "LOAD_STORE_ORDERS", "target": "APPEND_ALL_ORDERS"}, - {"source": "APPEND_ALL_ORDERS", "target": "SPLIT_BY_AMOUNT"}, - {"source": "SPLIT_BY_AMOUNT", "target": "OUTPUT_HIGH"}, - {"source": "SPLIT_BY_AMOUNT", "target": "OUTPUT_LOW"} + {"source": "LOAD_WEB_ORDERS", "target": "APPEND_ALL_ORDERS"}, + {"source": "LOAD_STORE_ORDERS", "target": "APPEND_ALL_ORDERS"}, + {"source": "APPEND_ALL_ORDERS", "target": "SPLIT_CUSTOMER_NAME"}, + {"source": "SPLIT_CUSTOMER_NAME", "target": "OUTPUT_ORDERS"} ] } } diff --git a/skills/explaining-batch-data-transform/references/bdt-function-catalog.md b/skills/explaining-batch-data-transform/references/bdt-function-catalog.md index 4f59160..7d986e9 100644 --- a/skills/explaining-batch-data-transform/references/bdt-function-catalog.md +++ b/skills/explaining-batch-data-transform/references/bdt-function-catalog.md @@ -34,7 +34,7 @@ |---|---|---| | `abs(n)` | number → number | Absolute value (strips sign). | | `ceiling(n)` | number → number | Round up, away from zero for negatives. | -| `floor(n)` | number → number | Round down, toward zero for negatives (or toward the next integer up for negatives). | +| `floor(n)` | number → number | Round toward negative infinity (down on the number line). For negatives, rounds away from zero (e.g., `floor(-2.3) = -3`). | | `exp(n)` | number → number | e raised to n. | | `log(base, n)` | number → number | Logarithm of n in the given base. | | `max(a, b, …)` | number → number | Largest value. | diff --git a/skills/explaining-batch-data-transform/references/bdt-node-catalog.md b/skills/explaining-batch-data-transform/references/bdt-node-catalog.md index 1f3312c..a1b4b85 100644 --- a/skills/explaining-batch-data-transform/references/bdt-node-catalog.md +++ b/skills/explaining-batch-data-transform/references/bdt-node-catalog.md @@ -533,23 +533,42 @@ unmapped source field is discarded. ## `action: "split"` — UI: "Split" -**Purpose.** Route rows into multiple downstream branches based on per-branch predicates. +**Purpose.** Split the value of one source string field into multiple target columns based on a delimiter. One row in, one row out — each row's `sourceField` is split into the named `targetFields`. **Key parameters** (class `SplitParametersInputRepresentation`): | Param | Type | Notes | |---|---|---| -| `branches` | array of `{ name, expression }` | Per-branch routing predicate. | +| `sourceField` | `string` | Name of the field whose value will be split. | +| `delimiter` | `string` | Delimiter used to split the source value. | +| `targetFields` | `{name, label}[]` | One entry per column the split produces. Order matches the left-to-right order of the split parts. | **Lineage effect.** -- Rows: partitioned across branches (each branch exposes the rows matching its predicate). -- Columns: same as input. +- Rows: unchanged. Row cardinality is preserved — split does not route rows into branches. +- Columns: adds each `targetFields[i].name` as a new column. The original `sourceField` passes through unchanged. **Gotchas.** -- A split node has one upstream source but *multiple* downstream consumers (each branch is a - logical output). When narrating lineage, mention which branch feeds which downstream node. +- `split` is string-splitting, not row-routing. If you need to route rows into multiple branches based on predicates, use `filter` nodes downstream of a common source, not `split`. +- If a row's `sourceField` has fewer delimited parts than the `targetFields` length, the remaining target columns are populated with NULL (no error). +- If a row's `sourceField` has more delimited parts than `targetFields` length, the extra parts are discarded. -**Source.** `SplitNodeInputRepresentation` + `SplitParametersInputRepresentation`. +**Example.** +```jsonc +{ + "action": "split", + "sources": ["LOAD_RAW"], + "parameters": { + "sourceField": "FullName__c", + "delimiter": " ", + "targetFields": [ + {"name": "FirstName__c", "label": "First Name"}, + {"name": "LastName__c", "label": "Last Name"} + ] + } +} +``` + +**Source.** `SplitNodeInputRepresentation` + `SplitParametersInputRepresentation` + `NameLabelInputRepresentation`. --- diff --git a/skills/explaining-batch-data-transform/tests/test_bdt_analyze.py b/skills/explaining-batch-data-transform/tests/test_bdt_analyze.py index 23df942..d360685 100644 --- a/skills/explaining-batch-data-transform/tests/test_bdt_analyze.py +++ b/skills/explaining-batch-data-transform/tests/test_bdt_analyze.py @@ -39,12 +39,10 @@ class TestBadInput(unittest.TestCase): def test_invalid_json_raises(self): p = FIXTURES / "_tmp_invalid.json" p.write_text("{not valid json") - try: - with self.assertRaises(bdt_analyze.BdtInputError) as cm: - bdt_analyze.DataTransform.from_path(p) - self.assertIn("Invalid JSON", str(cm.exception)) - finally: - p.unlink(missing_ok=True) + self.addCleanup(p.unlink, missing_ok=True) + with self.assertRaises(bdt_analyze.BdtInputError) as cm: + bdt_analyze.DataTransform.from_path(p) + self.assertIn("Invalid JSON", str(cm.exception)) def test_missing_nodes_raises(self): with self.assertRaises(bdt_analyze.BdtInputError):