bamaer commented on PR #8339:
URL: https://github.com/apache/hop/pull/8339#issuecomment-5644645462
Nice feature. My main concern is backwards compatibility: three of these
break pipelines that work on main and don't use the new syntax at all.
Serialization is fine — `ParameterField` is untouched and no
`@HopMetadataProperty` key changed, so existing XML loads identically. The
regressions are all behavioural.
## Blockers
**1. `getTableFields()` never migrated.** It still passes raw SQL to
`db.getQueryFields()` instead of `parseSqlParameterSpec(...).getPreparedSql()`
like every other call site. Any `?{name}` placeholder makes the driver reject
the statement; the exception is swallowed into `logError` and the method
returns null. Called from `DatabaseJoin.lookupValues()` on every pipeline start
and from `PipelineMeta` at design time.
**2. `parseSqlParameterSpec()` miscounts parameters.** It tracks only
single-quoted literals, so a `?` in a `--` / `/* */` comment or a `"quoted
identifier"` counts as positional. `data.keynrs` is now sized from this parse
rather than `meta.getParameters().size()`, so existing SQL fails with "field
not found: null" or a JDBC parameter-index error.
Both failure modes are silent in different ways. `check()` runs
`countParameters()` on the prepared SQL, so both scanners see the same string:
for a `?` in a `"quoted identifier"` they disagree (only `countParameters`
skips it) and you get a spurious mismatch remark on valid SQL; for a `?` in a
comment they agree, no remark appears, and `keynrs` is just quietly wrong.
JDBC can't do this parse: `?{name}` isn't valid JDBC, and
`getParameterMetaData()` returns only a count, never positions
(`Database.getParameterMetaData()` already catches `AbstractMethodError` and
falls back to manual counting). Usable as a cross-check, not as the parser.
Suggest one tokenizer in core returning the ordered reference list, with
`countParameters()` wrapping it. Needs to skip: `''` escaping,
`"`/backtick/`[]` identifiers, `--` and `/* */` comments, and PostgreSQL `$$`
quoting plus the `?`, `?|`, `?&` jsonb operators — that last one breaks real
pipelines.
**3. `getFields()` no longer binds incoming stream types.** This one affects
every existing transform, not just ones using `?{name}`:
- main: `param = getParameterRow(row)` — types from the **incoming row**,
values all NULL
- PR: `param = createMetadataLookupParameterRowMeta(...)` — types from the
**declared `ParameterField` list**, with dummy non-null values (`"metadata"`,
`0L`, …)
`createMetadataLookupParameterRowMeta()` never consults `row` and falls back
to `ValueMetaNone`. The type column in the positional parameter table is
commonly left unset, so existing transforms that relied on real stream types
now bind `ValueMetaNone` → `setNull(Types.VARCHAR)`. On the
`getQueryFieldsFallback` path that turns a working `WHERE num_col = ?` into
ORA-00932/ORA-01722 at design time.
If the declared types are needed for the named case, suggest preferring the
incoming row's type when the field resolves in `prev` and falling back to the
declared type only otherwise — and keeping NULL values rather than dummies.
## Should fix
**4. Stored-procedure deferral swallows connection failures.** In
`getFields()`, `db.connect()` is inside the try whose catch defers on
`isLikelyStoredProcedureSql()`. For any `exec`/`execute`/`{call` SQL, a dead
database or a genuine syntax error is discarded at `logDetailed` and
`getFields()` returns zero fields silently. Move `db.connect()` outside that
try.
**5. No validation when SQL is empty.** The closing brace before `// Look up
fields in the input stream` was removed, pulling that validation inside `if
(!Utils.isEmpty(sqlToUse))`. The `CouldNotReadFields` error is gone and a
transform that cannot run now reports nothing.
## Minor / follow-up
- The `data.parameterSpec == null ?
parseSqlParameterSpec(meta.getEffectiveSql(variables)) : ...` fallback in
`lookupValues()` is unreachable — `init()` always sets `data.parameterSpec`
first. Worth deleting rather than leaving: if it ever were reached it would
parse *unresolved* SQL while the statement was prepared from *resolved* SQL, so
the counts could disagree.
- `createUniqueOutputFieldName()` renames to `name_dbj1`, design-time
`RowMeta.renameValueMetaIfInRow` to `name_1` — same query, different field
names. `RowMeta.addValueMeta` already deduplicates, so the manual renaming
looks removable.
- `getMissingPositionalParameterFields()` checks only that a name is
declared, not that it exists in `prev`; and `check()` still adds "All fields
found" alongside a missing-field error. Only the first missing field is named.
- `getParameterRow()` is dead outside tests.
## GUI surface
`?{name}` is currently file-only — `DatabaseJoinDialog` still offers just
the positional parameter table — which conflicts with our rule against features
that exist only in a file.
This doesn't need an authoring UI. The SQL *is* the binding, so nothing
needs a second editing surface; a read-only panel under the SQL editor showing
what the parser resolved is enough:
| # | Placeholder | Input field | Type |
|---|---|---|---|
| 1 | `?{customer_id}` | customer_id | Integer |
| 2 | `?` | positional → row 1 | String |
| 3 | `?{region}` | **not found** | — |
`wSql` already has a `ModifyListener` and already calls `setPosition()` on
modify, so it's the same hook. And because it calls the same
`parseSqlParameterSpec()` the runtime uses, it can't drift — whatever it
displays is what executes. It would also have made issue 2 visible while
writing the SQL rather than at runtime.
Optional on top, both droppable: grey out the positional table when the SQL
has no bare `?`, and field-name autocomplete in the editor.
--
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]
