bamaer commented on PR #8632:
URL: https://github.com/apache/hop/pull/8632#issuecomment-5854408851
Nice, well-scoped feature and the tests are thorough. One blocker.
## Blocker
**`SampledValueLimits` replaces String/JSON values without respecting the
field's storage type, which breaks execution data registration for
lazy-converted fields.**
In `SampledValueLimits`, `decideString` computes the length correctly for
`STORAGE_TYPE_BINARY_STRING` (it handles the `byte[]`), but the replacement it
stores is always a `java.lang.String`. The row then holds a `String` in a field
whose `IValueMeta` says `String<binary-string>`. `ValueMetaBase.writeData`
casts to `byte[]` there, so `ExecutionData.getRowsBinaryGzipBase64Encoded()`
throws:
```
HopRuntimeException: body String<binary-string> : There was a data type
error:
the data type of java.lang.String object [<not stored, 16 characters>]
does not correspond to value meta [String<binary-string>]
```
That blob covers *all* data sets in the `ExecutionData`, so one such value
loses the whole registration for that tick. `LocalPipelineEngine`'s timer
catches it and logs `"Warning: unable to register execution data at location"`,
so from the user's side execution data logging just stops working. The Neo4j
location hits the same mismatch in `getNativeDataType` and writes `null` plus a
conversion error per value.
Reachable on the obvious case for this feature: CSV/Text File Input with
**Lazy conversion** on emits binary-string values, and those are exactly the
wide text/JSON pipelines the limits are for. Before this PR the samplers stored
`rowMeta.cloneRow(row)`, which kept the `byte[]`, so this is new.
`STORAGE_TYPE_INDEXED` has the same problem (a `String` where an `Integer`
index is expected), as does `decideJson`'s `TextNode` on a binary-storage JSON
field.
Suggested fix — build the marker from the storage type:
```java
private static Object marker(IValueMeta valueMeta, String text) {
return switch (valueMeta.getStorageType()) {
case IValueMeta.STORAGE_TYPE_BINARY_STRING ->
text.getBytes(StandardCharsets.UTF_8);
case IValueMeta.STORAGE_TYPE_INDEXED -> null; // cannot add an index
default -> valueMeta.getType() == IValueMeta.TYPE_JSON ?
TextNode.valueOf(text) : text;
};
}
```
A test with `meta.setStorageType(STORAGE_TYPE_BINARY_STRING)` going through
`getRowsBinaryGzipBase64Encoded()` would pin this down.
## Should fix
**`copyKept` silently stops cloning Timestamp values.** The switch in
`copyKept` lists STRING, JSON, BINARY, AVRO, DATE, NUMBER, INTEGER, BIGNUMBER,
BOOLEAN and falls through to `default -> value` for everything else.
`rowMeta.cloneRow` (the previous behaviour, and still the behaviour when
nothing is omitted) calls `cloneValueData` for every type. So as soon as one
cell in a row is dropped, a `TYPE_TIMESTAMP` cell in that same row is stored by
reference into the sample buffer instead of cloned —
`ValueMetaTimestamp.cloneValueData` does make a copy, and `Timestamp` is
mutable. Since the point of the switch is only to avoid cloning the cell you
are about to drop, `copyKept` can just be `valueMeta.cloneValueData(value)`
with no switch — identical to `cloneRow` for the kept cells.
**Large values are measured several times per row.**
`BasicDataProfilingDataSampler.sampleRow` calls
`getSampledValueLimits().omit(valueMeta, valueData)` per field, and each
`addSampleRow` -> `copyRow` re-runs `decide()` over every field again.
Measuring a JSON value means `JsonUtil.mapJsonToString(node, false)` —
materialising the whole document as a String — and measuring an Avro value
means a full `GenericDatumWriter` encode. With
min/max/minlength/maxlength/nulls/nonnulls that is up to ~5 `copyRow` calls per
field per row on top of the per-field `omit`, so a wide row with a JSON column
gets serialised many times per row. Computing the decisions once per row and
passing them to `copyRow` would remove it.
## Minor
- `parseLimit` maps a non-numeric or negative value to `null`, i.e.
*unlimited*. A typo like `1O000` silently turns the protection off, with no
warning anywhere. Worth a `log.logBasic`/`logError`, or GUI validation.
- `ExecutionDataProfile` gains ~70 lines of hand-written getters/setters.
Hop's contributor guide asks for Lombok `@Getter`/`@Setter` here.
## Verified locally
Built `core`, `engine` and `ui` on the PR head (`695a160b0a`);
`spotless:check` and `apache-rat:check` pass; `SampledValueLimitsTest` (12
tests) and the whole `org.apache.hop.execution.**` suite (58 tests) pass. The
blocker above was reproduced with a scratch test, not read off the diff. I did
not run the Hop GUI, so the editor layout and the new screenshot are unverified.
--
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]
