You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hop.apache.org by "rmannibucau (via GitHub)" <gi...@apache.org> on 2026/10/07 13:09:37 UTC
[PR] fix(tableinput): don't bind incoming rows when the SQL has no placeholders (hop)
rmannibucau opened a new pull request, #8787:
URL: https://github.com/apache/hop/pull/8787
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:
- 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 #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.
------------------------
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
- [ ] Run `mvn clean install apache-rat:check` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [X] If you have a group of commits related to the same change, please squash your commits into one and force push your branch using `git rebase -i`.
- [-] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable.
To make clear that you license your contribution under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [X] I hereby declare this contribution to be licensed under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [X] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tableinput): don't bind incoming rows when the SQL has no placeholders (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters commented on code in PR #8787:
URL: https://github.com/apache/hop/pull/8787#discussion_r4210072400
##########
plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.pipeline.transforms.tableinput;
+
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.ArgumentMatchers.notNull;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyInt;
+import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.util.UUID;
+import org.apache.hop.core.HopClientEnvironment;
+import org.apache.hop.core.database.Database;
+import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.row.IRowMeta;
+import org.apache.hop.core.row.RowMeta;
+import org.apache.hop.core.row.value.ValueMetaString;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.databases.h2.H2DatabaseMeta;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
+import org.apache.hop.pipeline.PipelineMeta;
+import org.apache.hop.pipeline.engines.local.LocalPipelineEngine;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for the Table Input query/parameter path.
+ *
+ * <p>These tests reproduce the regression where a hop into a Table Input whose SQL has no
+ * placeholders was still consumed as parameter rows in 2.20 (Issue #2722 / PR #8034): the incoming
+ * row was bound to a PreparedStatement without parameters, which fails on Oracle with ORA-17003 and
+ * on H2 with "Parameter index out of range".
+ *
+ * <p>The fix keeps the 2.20 "optional lookup" feature (collect parameter rows from any incoming
+ * hop, named parameters) while restoring the legacy behavior for SQL without placeholders: no
+ * collection, no bind, and the incoming hops are still drained so the sequence (header transform
+ * then extraction) keeps working.
+ */
+class TableInputTest {
+
+ private static final String CONNECTION_NAME = "table-input-test";
+
+ private Variables variables;
+ private TableInput tableInput;
+ private TableInputMeta meta;
+ private TableInputData data;
+ private Database db;
+
+ @BeforeAll
+ static void initHop() throws Exception {
+ HopClientEnvironment.init();
+ org.apache.hop.core.database.DatabasePluginType.getInstance()
+ .registerClassPathPlugin(H2DatabaseMeta.class);
+ }
+
+ @BeforeEach
+ void setUp() throws Exception {
+ variables = new Variables();
+
+ DatabaseMeta databaseMeta = connectionTo("mem:" + UUID.randomUUID());
+ // Force the H2 driver to load and validate the connection name before the transform uses it.
+ try (Database ignored =
+ new Database(
+ new org.apache.hop.core.logging.LoggingObject("TableInputTest"),
+ variables,
+ databaseMeta)) {
+ // connecting is enough
+ }
+
+ meta = new TableInputMeta();
+ meta.setConnection(CONNECTION_NAME);
+ meta.setUseNamedParameters(false);
+ meta.setSql("SELECT 1");
+
+ data = new TableInputData();
+ db = mock(Database.class);
+ data.db = db;
+ doReturn(new RowMeta()).when(db).getReturnRowMeta();
+ doReturn(null).when(db).getRow(any(ResultSet.class));
+ doReturn(databaseMeta).when(db).getDatabaseMeta();
+ doReturn(mock(ResultSet.class))
+ .when(db)
+ .openQuery(anyString(), any(IRowMeta.class), any(Object[].class), anyInt(), anyBoolean());
+
+ PipelineMeta pipelineMeta = new PipelineMeta();
+ MemoryMetadataProvider metadataProvider = new MemoryMetadataProvider();
+ metadataProvider.getSerializer(DatabaseMeta.class).save(databaseMeta);
+ pipelineMeta.setMetadataProvider(metadataProvider);
+ TransformMeta transformMeta = new TransformMeta("table input", meta);
+ pipelineMeta.addTransform(transformMeta);
+ // dispatch() in the BaseTransform constructor walks previous/next transforms; none exist here.
+
+ tableInput =
+ spy(
+ new TableInput(
+ transformMeta, meta, data, 1, pipelineMeta, spy(new LocalPipelineEngine())));
+ doReturn(transformMeta).when(tableInput).getTransformMeta();
+ doReturn(false).when(tableInput).isRowLevel();
+ doReturn(false).when(tableInput).isDebug();
+ doNothing().when(tableInput).logDetailed(any());
+ doNothing().when(tableInput).logBasic(any());
+ doNothing().when(tableInput).logDebug(any());
+ doNothing().when(tableInput).logError(any());
+ // 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());
Review 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.
##########
plugins/transforms/tableinput/src/main/java/org/apache/hop/pipeline/transforms/tableinput/TableInput.java:
##########
@@ -98,9 +98,32 @@ public boolean processRow() throws HopException {
if (isDetailed()) {
logDetailed("Reading all parameter rows from incoming hops");
}
- RowMetaAndData assembled = readAllParameterRows();
- parameters = assembled.getData();
- parametersMeta = assembled.getRowMeta();
+
+ // Legacy sequencing contract (2.19): when there is no info stream (empty lookup) and no
+ // named parameters, the incoming rows are not parameters at all. They are only consumed
+ // so an upstream "header" transform can complete before the extraction runs. Consuming
+ // them as parameter rows would bind them to a statement without placeholders (ORA-17003 on
+ // Oracle). Drain them without collecting.
+ //
+ if (Utils.isEmpty(meta.getLookup()) && !meta.isUseNamedParameters()) {
+ String resolved = resolveSql();
Review 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.
##########
docs/hop-user-manual/modules/ROOT/pages/pipeline/transforms/tableinput.adoc:
##########
@@ -271,8 +277,8 @@ On the *Fields* tab you can define the output schema instead of asking the datab
|SQL tab|SQL statement, optional SQL file, `Get SQL select statement`, `Use named parameters`, and `Insert field...` for `{openfield}fieldName{closefield}`.
|Use named parameters|When enabled, `{openfield}fieldName{closefield}` in SQL is bound to an incoming field. Off for existing Table Input metadata, on for new transforms. Disable if curly braces are used for something else. Checking this option also enables *Execute for each row* when hops exist, and selects the incoming transform when there is only one.
|Replace variables in script?|Enable to substitute variables (e.g., `+${param}+`) in your SQL before execution.
-|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.
Review 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.
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tableinput): don't bind incoming rows when the SQL has no placeholders (hop)
Posted by "rmannibucau (via GitHub)" <gi...@apache.org>.
rmannibucau commented on code in PR #8787:
URL: https://github.com/apache/hop/pull/8787#discussion_r4212242178
##########
plugins/transforms/tableinput/src/test/java/org/apache/hop/pipeline/transforms/tableinput/TableInputTest.java:
##########
@@ -0,0 +1,262 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hop.pipeline.transforms.tableinput;
+
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.isNull;
+import static org.mockito.ArgumentMatchers.notNull;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyInt;
+import static org.mockito.Mockito.anyString;
+import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.util.UUID;
+import org.apache.hop.core.HopClientEnvironment;
+import org.apache.hop.core.database.Database;
+import org.apache.hop.core.database.DatabaseMeta;
+import org.apache.hop.core.row.IRowMeta;
+import org.apache.hop.core.row.RowMeta;
+import org.apache.hop.core.row.value.ValueMetaString;
+import org.apache.hop.core.variables.Variables;
+import org.apache.hop.databases.h2.H2DatabaseMeta;
+import org.apache.hop.metadata.serializer.memory.MemoryMetadataProvider;
+import org.apache.hop.pipeline.PipelineMeta;
+import org.apache.hop.pipeline.engines.local.LocalPipelineEngine;
+import org.apache.hop.pipeline.transform.TransformMeta;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Unit tests for the Table Input query/parameter path.
+ *
+ * <p>These tests reproduce the regression where a hop into a Table Input whose SQL has no
+ * placeholders was still consumed as parameter rows in 2.20 (Issue #2722 / PR #8034): the incoming
+ * row was bound to a PreparedStatement without parameters, which fails on Oracle with ORA-17003 and
+ * on H2 with "Parameter index out of range".
+ *
+ * <p>The fix keeps the 2.20 "optional lookup" feature (collect parameter rows from any incoming
+ * hop, named parameters) while restoring the legacy behavior for SQL without placeholders: no
+ * collection, no bind, and the incoming hops are still drained so the sequence (header transform
+ * then extraction) keeps working.
+ */
+class TableInputTest {
+
+ private static final String CONNECTION_NAME = "table-input-test";
+
+ private Variables variables;
+ private TableInput tableInput;
+ private TableInputMeta meta;
+ private TableInputData data;
+ private Database db;
+
+ @BeforeAll
+ static void initHop() throws Exception {
+ HopClientEnvironment.init();
+ org.apache.hop.core.database.DatabasePluginType.getInstance()
+ .registerClassPathPlugin(H2DatabaseMeta.class);
+ }
+
+ @BeforeEach
+ void setUp() throws Exception {
+ variables = new Variables();
+
+ DatabaseMeta databaseMeta = connectionTo("mem:" + UUID.randomUUID());
+ // Force the H2 driver to load and validate the connection name before the transform uses it.
+ try (Database ignored =
+ new Database(
+ new org.apache.hop.core.logging.LoggingObject("TableInputTest"),
+ variables,
+ databaseMeta)) {
+ // connecting is enough
+ }
+
+ meta = new TableInputMeta();
+ meta.setConnection(CONNECTION_NAME);
+ meta.setUseNamedParameters(false);
+ meta.setSql("SELECT 1");
+
+ data = new TableInputData();
+ db = mock(Database.class);
+ data.db = db;
+ doReturn(new RowMeta()).when(db).getReturnRowMeta();
+ doReturn(null).when(db).getRow(any(ResultSet.class));
+ doReturn(databaseMeta).when(db).getDatabaseMeta();
+ doReturn(mock(ResultSet.class))
+ .when(db)
+ .openQuery(anyString(), any(IRowMeta.class), any(Object[].class), anyInt(), anyBoolean());
+
+ PipelineMeta pipelineMeta = new PipelineMeta();
+ MemoryMetadataProvider metadataProvider = new MemoryMetadataProvider();
+ metadataProvider.getSerializer(DatabaseMeta.class).save(databaseMeta);
+ pipelineMeta.setMetadataProvider(metadataProvider);
+ TransformMeta transformMeta = new TransformMeta("table input", meta);
+ pipelineMeta.addTransform(transformMeta);
+ // dispatch() in the BaseTransform constructor walks previous/next transforms; none exist here.
+
+ tableInput =
+ spy(
+ new TableInput(
+ transformMeta, meta, data, 1, pipelineMeta, spy(new LocalPipelineEngine())));
+ doReturn(transformMeta).when(tableInput).getTransformMeta();
+ doReturn(false).when(tableInput).isRowLevel();
+ doReturn(false).when(tableInput).isDebug();
+ doNothing().when(tableInput).logDetailed(any());
+ doNothing().when(tableInput).logBasic(any());
+ doNothing().when(tableInput).logDebug(any());
+ doNothing().when(tableInput).logError(any());
+ // 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());
Review Comment:
updated the test but small side note: `any()` *does* match `null` (not the other signature)
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(tableinput): don't bind incoming rows when the SQL has no placeholders (hop)
Posted by "mattcasters (via GitHub)" <gi...@apache.org>.
mattcasters merged PR #8787:
URL: https://github.com/apache/hop/pull/8787
--
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: issues-unsubscribe@hop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org