mattcasters commented on code in PR #8339:
URL: https://github.com/apache/hop/pull/8339#discussion_r4028643229
##########
plugins/transforms/databasejoin/src/main/java/org/apache/hop/pipeline/transforms/databasejoin/DatabaseJoinMeta.java:
##########
@@ -414,33 +665,32 @@ public void check(
@Override
public IRowMeta getTableFields(IVariables variables) {
- // Build a dummy parameter row...
- //
-
DatabaseMeta databaseMeta =
getParentTransformMeta().getParentPipelineMeta().findDatabase(connection,
variables);
- IRowMeta param = new RowMeta();
- for (ParameterField field : this.parameters) {
- IValueMeta v;
- try {
- int id = ValueMetaFactory.getIdForValueMeta(field.getType());
- v = ValueMetaFactory.createValueMeta(field.getName(), id);
- } catch (HopPluginException e) {
- v = new ValueMetaNone(field.getName());
- }
- param.addValueMeta(v);
- }
-
IRowMeta fields = null;
if (databaseMeta != null) {
Database db = new Database(loggingObject, variables, databaseMeta);
databases = new Database[] {db}; // Keep track of this one for
cancelQuery
+ SqlParameterSpec parameterSpec = null;
try {
+ parameterSpec = parseSqlParameterSpec(resolveSql(variables));
db.connect();
- fields = db.getQueryFields(resolveSql(variables), true, param, new
Object[param.size()]);
+ IRowMeta param = createMetadataLookupParameterRowMeta(parameterSpec);
+ fields =
+ db.getQueryFields(
+ parameterSpec.getPreparedSql(),
+ true,
+ param,
+ createMetadataLookupParameterRowData(param));
} catch (HopException dbe) {
+ if (parameterSpec != null &&
isLikelyStoredProcedureSql(parameterSpec.getPreparedSql())) {
Review Comment:
**[suggestion]** `getTableFields()` still runs `db.connect()` inside the try
that defers on `isLikelyStoredProcedureSql()`. For `exec` / `execute` / `{call`
SQL, a dead database or a genuine syntax error is logged at detailed/debug and
the method returns null. `getFields()` was fixed by moving `connect()` out of
that catch; this path was not.
**Suggestion:** Connect first; only defer metadata discovery after a
successful connection when the query itself looks like a stored-procedure call.
##########
plugins/transforms/databasejoin/src/test/java/org/apache/hop/pipeline/transforms/databasejoin/DatabaseJoinMetaTest.java:
##########
@@ -191,37 +193,144 @@ void getFieldsDoesNothingWhenConnectionIsMissing()
throws Exception {
Assertions.assertEquals("id", row.getValueMeta(0).getName());
}
+ void parseSqlParameterSpecSupportsMixedNamedAndPositionalPlaceholders() {
Review Comment:
**[suggestion]**
`parseSqlParameterSpecSupportsMixedNamedAndPositionalPlaceholders` has no
`@Test`, so the PR description’s headline mixed `?{customer_id}` + `?` example
never runs.
**Suggestion:** Add `@Test` (and keep it green after the parser fixes).
##########
core/src/main/java/org/apache/hop/core/database/Database.java:
##########
@@ -108,4800 +109,5123 @@
* The database specific parameters are defined in DatabaseInfo.
*/
public class Database implements IVariables, ILoggingObject, AutoCloseable {
- private static final Class<?> PKG = Database.class;
-
- private static final Map<String, Set<String>> registeredDrivers = new
HashMap<>();
+ private static final Class<?> PKG = Database.class;
Review Comment:
**[suggestion]** The entire `Database` class body was re-indented to 4
spaces. This repository formats Java with google-java-format (2-space indent)
via Spotless. The PR’s ~5000-line `Database.java` churn is almost entirely
whitespace, will fail `spotless:check` or be rewritten on `spotless:apply`
during compile, and will conflict with every other `Database.java` change on
main.
**Suggestion:** Revert the whitespace-only rest of `Database.java` and add
the parser in the existing 2-space style so the functional delta is reviewable.
--
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]
