mattcasters commented on PR #8414:
URL: https://github.com/apache/hop/pull/8414#issuecomment-5710664232
Re-checked the original review items against `b262755` (code/tests in
`1d244e92`, dimension contract documented in the latest commit). All seven are
addressed.
**Bugs**
1. **Lazy String → Vector.** `toVector` no longer converts through the
target Vector's `storageMetadata`. `TYPE_STRING` uses
`parse(meta2.getString(data2))`; a Vector field's own lazy bytes use
`meta2.convertBinaryStringToNativeType`. Covered by
`convertsALazilyConvertedStringFieldToVector`.
The note that this was latent in Select Values is correct:
`cloneValueMeta` copies `storageMetadata`, so the NPE did not show up on that
path. The unit test still uses a fresh Vector with `storageMetadata == null`,
which is the broken case, and that is the right regression test.
2. **`cloneValueData`.** Non-`NORMAL` storage is left alone, matching
`ValueMetaBase`. A Copy hop on a lazy Vector field will no longer materialise
`byte[]` into `float[]`. Covered by
`cloneLeavesLazilyConvertedValuesInTheirBinaryForm`.
**Suggestions**
3. `writeData` converts first and writes the null flag from `vector == null`.
4. `getValueFromResultSet` handles `float[]`, `double[]`, `String` and
`java.sql.Array` (including `Number[]`) before falling back to `toString()`.
5. `catch (RuntimeException)` is now `ClassCastException` with the cause
attached; `NullPointerException` is no longer swallowed.
6. `typeCompare` uses `Arrays.compare`; the javadoc matches the
lexicographic order.
7. The four requested tests are there: lazy String source, clone of lazy
bytes, factory id `1536`, prepared statement binds the canonical text. Locally
`ValueMetaVectorTest` is 23/23 on this head. The value-types intro also lists
Vector as a real type now.
**Nits, not blockers**
- IT `0116-vector-lazy-conversion` is mislabeled: the description talks
about a lazy String field converting to Vector, but the pipeline types the CSV
field as Vector and converts Vector → String. That covers the clone/lazy Vector
neighbourhood, not bug 1. Bug 1 is the unit test.
- `testCompareOrdersByLengthThenElement` still has the old name, and `[1]`
vs `[1,2]` would pass under both the old length-first order and
`Arrays.compare`.
- `Float[]` / `Double[]` as a raw `ResultSet.getObject()` still go through
`toString()`. Typical JDBC array columns come back as `java.sql.Array` and are
handled.
Looks good to merge from this side.
--
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]
