Repository navigation
fix(tableinput): don't bind incoming rows when the SQL has no placeholders - #8787
Conversation
…lders PR apache#8034 (Issue apache#2722) made Table Input read parameter rows from every incoming hop, even with an empty "Insert data from transform". Pipelines that feed a Table Input for sequencing only (e.g. a header transform before the extraction) now had their header row bound to the statement even though the SQL has no bind variable, failing on Oracle with ORA-17003 "Invalid column index" and on H2 with "Parameter index out of range". Fix both sides of the regression while keeping the 2.20 feature: - TableInputSql: count real positional placeholders (?) outside string literals, quoted identifiers, comments and Hop variables. prepare() with named parameters disabled and bind() now return a Bound with null parameter metadata/data when the SQL has no placeholder, so Database runs a plain statement instead of a PreparedStatement. - TableInput: in the assemble-all branch, when the lookup is empty and named parameters are disabled and the SQL has no placeholder, drain the incoming rows without collecting or binding them. Upstream transforms still complete (sequencing contract of 2.19) and consumesMainInput() stays true so the unconsumed-main-input check keeps passing. - Named and positional parameters are still read from every incoming hop (optional lookup, PR apache#8034) when the SQL declares them. Tests: new TableInputTest with an in-memory H2 connection reproduces the failure before the fix (openQuery receives the header row on "SELECT 1") and guards named/positional binding, literal '?' handling and the legacy drain-without-bind path. TableInputSqlTest updated accordingly. Documentation: tableinput.adoc now describes the no-placeholder behavior and the 2.19/2.20 compatibility.
| |Insert data from transform|Optional. Names the hop that should be treated as informational (parameter rows, not the SQL result). Parameter values are read from **all** incoming hops. Leave empty when you only need the connected hops as parameter sources. | ||
| |Execute for each row?|When incoming hops exist, run the SQL query once for each incoming row. Disable to concatenate all incoming rows into a single parameter list (legacy `WHERE bar IN (?,?,?)` from several one-field rows). Incoming hops with different layouts still produce the mixed-layout warning. | ||
| |Insert data from transform|Optional. Names the hop that should be treated as informational (parameter rows, not the SQL result). Parameter values are read from **all** incoming hops. Leave empty when you only need the connected hops as parameter sources. When the SQL has no bind placeholder, the incoming rows are only drained, never bound. | ||
| |Execute for each row?|When incoming hops exist, run the SQL query once for each incoming row. Disable to concatenate all incoming rows into a single parameter list (legacy `WHERE bar IN (?,?,?)` from several one-field rows). Incoming hops with different layouts still produce the mixed-layout warning. When the SQL has no bind placeholder, the incoming rows are only drained, never bound, and the query runs once without parameters. |
There was a problem hiding this comment.
[suggestion] This sentence says that with no bind placeholder the incoming rows are only drained and the query runs once. That is only the assemble-all branch (executeEachInputRow off, empty lookup, named parameters off) in TableInput.processRow.
When Execute for each row is on, that branch is skipped. The first row still calls doQuery, and determineDoneReading calls doQuery again for every later row. TableInputSql.prepare drops the bind, so there is no ORA-17003, but a multi-row predecessor repeats the whole result set. Lines 88–89 say the same thing ("only drained", "before the query runs").
Suggestion: Limit the "drained / runs once" wording to Execute for each row being off. If a single execution is the intended contract for every mode, also drain without re-querying when countPositionalPlaceholders is 0.
| // Oracle). Drain them without collecting. | ||
| // | ||
| if (Utils.isEmpty(meta.getLookup()) && !meta.isUseNamedParameters()) { | ||
| String resolved = resolveSql(); |
There was a problem hiding this comment.
[suggestion] resolveSql() is invoked here, outside the try/catch that doQuery uses for the same call. A missing SQL file (or any other HopException from getEffectiveSql) used to be logged as "Could not get SQL", with setErrors(1), stopAll(), and processRow returning false. On this new path the exception leaves processRow and the engine reports it as an unexpected error. This only happens when the lookup is empty and named parameters are off, which is the path this fix adds.
Suggestion: Catch HopException around this call the same way doQuery does, and return false before draining.
| // doQuery() reads through data.db; make the query "open" successfully with no rows. | ||
| doReturn(mock(ResultSet.class)) | ||
| .when(db) | ||
| .openQuery(anyString(), any(), any(), anyInt(), anyBoolean()); |
There was a problem hiding this comment.
[suggestion] Mockito 5's any() and any(IRowMeta.class) do not match null. The no-placeholder path calls openQuery(sql, null, null, ...), so both stubs in setUp miss, the mock returns null, and doQuery takes the "Couldn't open Query" path (setErrors(1), stopAll()). The verify calls still pass, so these tests never show that the transform succeeds. The comment above this stub says the query opens successfully.
legacyEmptyLookupDrainsWithoutCollectingOrBinding is the same scenario as sqlWithoutParametersDoesNotBindIncomingRows.
Suggestion: Stub with nullable(IRowMeta.class) and nullable(Object[].class), assert that processRow() returns true and getErrors() stays 0, and drop the duplicate test.
There was a problem hiding this comment.
updated the test but small side note: any() does match null (not the other signature)
- TableInput: catch HopException around resolveSql() in the drain branch (empty lookup, named parameters off) so a missing SQL file is reported as "Could not get SQL" with setErrors(1) + stopAll() instead of an unexpected engine error, mirroring doQuery(). - Docs: qualify the drain-without-bind wording with *Execute for each row* off; with it on the query runs once per incoming row, still with no parameter bound. - Tests: single nullable(IRowMeta/Object[]) openQuery stub (matches the null-params path), assertSucceeded() (errors == 0, no stopAll()), drop the duplicate legacyEmptyLookup test.
2 participants
PR #8034 (Issue #2722) made Table Input read parameter rows from every incoming hop, even with an empty "Insert data from transform". Pipelines that feed a Table Input for sequencing only (e.g. a header transform before the extraction) now had their header row bound to the statement even though the SQL has no bind variable, failing on Oracle with ORA-17003 "Invalid column index" and on H2 with "Parameter index out of range".
Fix both sides of the regression while keeping the 2.20 feature:
Tests: new TableInputTest with an in-memory H2 connection reproduces the failure before the fix (openQuery receives the header row on "SELECT 1") and guards named/positional binding, literal '?' handling and the legacy drain-without-bind path. TableInputSqlTest updated accordingly. Documentation: tableinput.adoc now describes the no-placeholder behavior and the 2.19/2.20 compatibility.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean install apache-rat:checkto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.git rebase -i.addresses #123), if applicable.To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.