bamaer commented on PR #8428:
URL: https://github.com/apache/hop/pull/8428#issuecomment-5712535360
@mattcasters thanks, all twelve items are addressed. Both dialogs are now
generated from `@GuiWidgetElement` on `GuiWidgetGroupType.TABS`, with the
filters and column-mapping TableViews registered through `registerExtraGroup`,
following `ParquetOutputDialog`. The search dialog went from 386 to 227 lines
and the upsert one from 396 to 232.
On commit size I took the Table Output semantics: 0 means one transaction
for the whole run rather than JDBC autocommit, and batching is disabled
whenever an error hop is attached, because a rejected batch cannot be mapped
back onto the rows that caused it. Rows then go one statement at a time behind
a savepoint, so a rejected row is diverted while the transaction survives.
Commit size, embedding dimensions, top-k and minimum score are all resolvable
strings now.
Reviewing the finished diff turned up six more problems, so those are fixed
here too:
1. The `"doc_null"` key you found had a mirror image: a null document ID
with a valid chunk index produced the literal key `"null_3"`. A chunk index
repeats across documents, so the document ID is now required and the row is
rejected without one.
2. Batching was disabled via `isUseSafePoints()`, which is only true for
`PostgreSqlDatabaseMeta`. On a Generic database connection with an error hop,
batching stayed on and a failed batch dropped its rows silently. It is now
gated on `isDoingErrorHandling()` directly.
3. `deleteDocument()` ran without a savepoint, so a failed delete aborted
the transaction and every following row failed too.
4. `check()` reported a hard error when a variable was used for dimensions,
commit size or top-k, which is exactly the usage item 2 asked for. It now skips
values that still hold `${...}`, using `StringUtil.containsVariableToken`.
5. `CREATE EXTENSION` ran on every `ensureSchema` call once index and column
creation became independent of table creation. It is back inside the
create-table branch, where it is the only thing that needs it.
6. The concurrent-DDL error was raised only for table creation, although
`CREATE INDEX` and `ALTER TABLE ADD COLUMN` now run independently.
Plus three smaller ones: the schema DDL ran inside the data transaction, so
a rollback of one failed row could undo the created table; the per-document
delete cache evicted its oldest entry when full, which let the same document be
deleted twice, taking with it the chunks the run had already written; and a
catch-and-rethrow that did nothing.
Coverage: 51 unit tests and five integration tests. The new ITs cover the
rejected row, savepoint recovery from a refused delete (a trigger in the
container refuses deletes for one document, so without the savepoint the next
document is rejected too), and the variable-driven options. The two design-time
checks run through `hop lint`, which invokes the same `checkTransforms()` the
GUI does. I verified every fix by reverting it and confirming the matching test
fails.
One correction to my own reasoning while reviewing: I initially thought the
`CREATE EXTENSION` change fixed a privilege problem. It does not. I checked
against a real `pgvector/pgvector:pg16` container and `CREATE EXTENSION IF NOT
EXISTS` succeeds for a non-superuser when the extension is already present,
which it always is on a table that has a vector column. The change removes a
redundant statement rather than a failure, so there is no integration test for
it.
--
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]
