bamaer commented on PR #8275: URL: https://github.com/apache/hop/pull/8275#issuecomment-5571342692
Thanks for this — nice, complete piece of work. Meta + data + transform + dialog + docs + unit tests + a golden-dataset integration test wired into `main-0019` is exactly the scope I like to see in a transform PR. Metadata injection is wired correctly (`@HopMetadataProperty` with injection key and description), the loadSave test was updated, and `all-transforms.hpl` was regenerated. Backward compatibility is handled properly: `setDefault()` sets `enclosure = ""` and the new path is guarded by `Utils.isEmpty(data.enclosure)`, so pipelines that don't set an enclosure take the exact old `Pattern.split(…, -1)` path. Pipelines whose XML predates the field deserialize to `null` → `""` → legacy path. No regression for anyone not using the feature. My concern is the foundation you're building on. `Const.splitString(…)` is a split-then-reconcatenate heuristic whose own javadoc says it *"expects that the data contains an even number of enclosure strings in the input; otherwise the results are undefined."* In a transform facing arbitrary user data, "undefined" turns into silently dropped rows. **Bottom line:** the feature and the approach are right; I'd just like the two findings below sorted before we merge. Happy to help with either. --- ## 1. Rows are silently lost on stray or unbalanced enclosures https://github.com/apache/hop/blob/3a92ea02575491a51ae4f8bd90607487269522c9/plugins/transforms/splitfieldtorows/src/main/java/org/apache/hop/pipeline/transforms/splitfieldtorows/SplitFieldToRows.java#L117-L129 Executed against `hop-core` on this branch, delimiter `,`, enclosure `"`: | input | this PR | without enclosure | |---|---|---| | `a"b,c` | **0 rows** | `a"b`, `c` | | `a,"b` | `a` — the `"b` vanishes | `a`, `"b` | | `,` | **0 rows** | ``, `` | One stray quote in one input row makes the whole row disappear — no error, no log line, no error-handling path. Inside `Const.splitString` the unterminated `concatSplit` buffer is simply never flushed when the loop ends. That's data loss rather than a parse quirk, which is why I'd rather not let it through. ## 2. Trailing empty values are dropped once an enclosure is set `a,b,,` yields 2 rows with an enclosure and 4 without, because `Const.splitString` uses `String.split(...)` at the default limit 0. The javadoc you added on `splitSource` acknowledges that the `-1` behaviour is kept only on the *else* branch — but nothing in the dialog or the docs tells users that filling in Enclosure quietly changes empty-value semantics. --- ## Suggested fix Both findings come from `Const.splitString` being a poor fit here. Replacing that one call with an explicit scanner resolves them together: ```java private String[] splitSource(String originalString) { if (Utils.isEmpty(data.enclosure) || meta.isIsDelimiterRegex()) { // use -1 to include trailing empty strings return data.delimiterPattern.split(originalString, -1); } return splitWithEnclosure(originalString); } /** Split on the delimiter, ignoring delimiters inside enclosures. Doubled enclosures * inside an enclosed value are kept as one literal enclosure. Trailing empty values * are preserved, matching the non-enclosure behaviour. */ private String[] splitWithEnclosure(String source) { String delimiter = data.delimiter; String enclosure = data.enclosure; List<String> values = new ArrayList<>(); StringBuilder value = new StringBuilder(); boolean inEnclosure = false; int index = 0; while (index < source.length()) { if (source.startsWith(enclosure, index)) { if (inEnclosure && source.startsWith(enclosure, index + enclosure.length())) { value.append(enclosure); index += 2 * enclosure.length(); } else { inEnclosure = !inEnclosure; index += enclosure.length(); } } else if (!inEnclosure && !delimiter.isEmpty() && source.startsWith(delimiter, index)) { values.add(value.toString()); value.setLength(0); index += delimiter.length(); } else { value.append(source.charAt(index)); index++; } } if (inEnclosure) { logError(BaseMessages.getString(PKG, "SplitFieldToRows.Log.UnterminatedEnclosure", source)); } values.add(value.toString()); return values.toArray(new String[0]); } ``` Needs `java.util.ArrayList` / `java.util.List` imports and one message key: ```properties SplitFieldToRows.Log.UnterminatedEnclosure=Unterminated enclosure in value [{0}], the remainder was treated as one value. ``` This also drops the `splitStrings != null ? … : new String[] {originalString}` branch, which is unreachable — `originalString` is set to `""` a few lines above, and `Const.splitString` returns `null` only for `null` input. I ran the replacement against every case above: | input | before | after | |---|---|---| | `hi,"hello, world","hey"` | 3 rows ✓ | 3 rows ✓ | | `"a","b","c"` | 3 rows ✓ | 3 rows ✓ | | `a"b,c` | 0 rows | `ab,c` + warning | | `a,"b` | `a` | `a`, `b` + warning | | `,` | 0 rows | ``, `` | | `a,b,,` | 2 rows | 4 rows | | `"a,b","c""d"` | `a,b`, `c""d` | `a,b`, `c"d` | No row is ever lost, trailing empties match the non-enclosure path, and malformed input is logged instead of vanishing. Your existing tests keep the same expectations. (The last row is a free side effect — doubled enclosures now unescape per RFC4180.) **Tests worth adding:** trailing empties (`a,b,,` → 4 rows), unterminated enclosure, and `${VAR}` substitution in the enclosure — the code resolves it, nothing exercises it yet. A couple of adjacent things I noticed are really pre-existing or cross-transform concerns rather than yours to fix here (how `Split fields` handles the same problem, and a `check()` remark when an enclosure is combined with a regex delimiter). I'll raise those separately. --- *Checked out and built locally on top of current main: `spotless:check` clean, 10/10 unit tests pass. The behaviour above was verified by executing `Const.splitString` and the proposed replacement directly.* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
