Skip to content

Enhance DatabaseJoin with SQL parameter support - #8339

Open
beatum wants to merge 7 commits into
apache:mainfrom
beatum:main
Open

Enhance DatabaseJoin with SQL parameter support#8339
beatum wants to merge 7 commits into
apache:mainfrom
beatum:main

Conversation

@beatum

@beatum beatum commented Sep 12, 2026

Copy link
Copy Markdown

Database Join now supports named placeholders in SQL using:

?{fieldName}

You can also mix named and positional placeholders in the same SQL.

Example:

SELECT order_id, total
FROM orders
WHERE customer_id = ?{customer_id}
  AND status = ?

How binding works:

?{customer_id} binds by incoming field name (customer_id).

? binds by positional parameter mapping from the step parameter grid (existing behavior).
SQL is converted to prepared-statement form before execution.

@bamaer

bamaer commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

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 ValueMetaNonesetNull(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.

@beatum

beatum commented Sep 14, 2026

Copy link
Copy Markdown
Author

Thanks for your time and patience. I will continue learning and improving throughout this project.
Here is my latest update. 😊

1.Database.java

  • a. deidcated function SqlParameterSpec which can support for JDBC parameter marker also ?{name}

2.DatabaseJoin.java

  • a. Removed unreachable fallback in lookupValues():

    • no longer reparses SQL from meta.getEffectiveSql(...);

    • uses data.parameterSpec directly and throws explicit error if unexpectedly null.

  • b. Removed custom runtime output field-name dedupe helpers isOutputFieldNameInUse and createUniqueOutputFieldName respectively.

  • c.Runtime metadata appending now sets base field name and relies on RowMeta.addValueMeta() for deduping, aligning naming behavior with design- timeflow.

3. DatabaseJoinMeta.java

  • a. parseSqlParameterSpec(...) now delegates to Database.parseSqlParameterSpec(...) single parser source of truth.

  • b. getTableFields(...) now uses parsed/prepared SQL getPreparedSql() instead of raw SQL, so ?{name} never goes directly to JDBC.

  • getFields(...) metadata parameter typing updated:

    • prefer incoming stream type when field exists in prev;

    • fallback to declared ParameterField type otherwise;

    • keep parameter row values null (removed dummy typed values).

  • c. Stored-procedure handling tightened:

    • in getFields(), connection failures are no longer swallowed by stored-proc deferral.

    • in getTableFields(), stored-proc metadata probe failures are deferred detailed/debuginstead of hard error logging.

  • d. check(...) now reports error when SQL is empty ,instead of silently skipping query validation.

  • Positional parameter missing checks improved:

    • now verifies both declaration and existence in incoming row;

    • reports missing positional field names clearly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants