bamaer commented on PR #8244:
URL: https://github.com/apache/hop/pull/8244#issuecomment-5601951310
Nice cleanup overall — the timestamp mask fix, the `TYPE_TIMESTAMP` case in
StreamLookup, the try-with-resources on `getColumns()`, and routing ordinary
JDBC away from the `SOURCE_DATA_TYPE` switch all look right, and the
`totalDigits()` split does what it claims (characterization goldens drop 62 →
15 divergences).
One blocker before this can go in:
**Error dialog is swallowed on "Get Fields"** —
`GetQueryFieldsProgressDialog.java:75` + `ProgressMonitorDialog.java:194`
The new `monitor.done()` in the runnable's own `finally` disposes the
progress shell while the `InvocationTargetException` is still propagating —
before `runMonitoredWork` reaches its `catch` and assigns `targetException`.
`pumpDisplayUntilShellDisposed()` only checks the exception fields at the top
of the loop, so it exits on `shell.isDisposed()` and returns normally:
`showErrorDialog()` never fires and `open()` returns null. A bad query in Table
Input's Get Fields now fails silently.
Suggested fix: re-check `interruptedException`/`targetException` after the
loop, and drop the added `monitor.done()` — the new `finally { dispose(); }` in
`ProgressMonitorDialog` already covers the hang it was guarding against.
Two more I'd fix here since they're small:
- `PostgreSqlDatabaseMeta.java:480` — the switch to `NUMERIC(length,
precision)` is the right call (the old `length+precision` inflated scale on
every round trip), but `length >= precision` isn't guaranteed the way
`length+precision >= precision` was, so scale > precision can emit DDL that
PostgreSQL < 15 rejects. The new test asserts `NUMERIC(5, 7)`, which is exactly
that case. Worth a clamp plus a test change. The narrowing of hand-authored
`Number(10,3)` → `NUMERIC(10,3)` is correct per Hop semantics but changes
generated DDL for existing users — worth a release note.
- `Database.java:2295` — `getQueryFieldsFallback` is called inside the `try`
and again in the `catch`, so a failing fallback runs the user's SQL twice.
Non-blocking, happy to see these as follow-ups:
- `MsSqlServerDatabaseMeta.java:81` — `originalColumnType` is checked before
the Hop type, so a DATE column explicitly converted to Timestamp still
generates `DATE` and loses the time. Swapping the two checks would fix it.
- `Database.java:3485` — the explicit `Types.NUMERIC → ValueMetaInteger`
mapping is gone; drivers reporting precision 0 now yield a double-backed
`ValueMetaNumber`.
- `DatabaseMeta.java:1098` — dropping the `supportsCatalogs()` guard changes
behavior for the four dialects that return false (MSSQL, Access, Gupta, Iris).
I think it's a net improvement (`mydb.dbo` used to quote as the broken
`[mydb.dbo]`), but it does mean a schema with a literal dot now splits. Just
confirming that's intentional.
--
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]
