xiangfu0 commented on PR #17939:
URL: https://github.com/apache/pinot/pull/17939#issuecomment-5853046653
Rebased onto latest master (clean, no conflicts) and addressed all 16 open
review threads:
**Security / trust boundary**
- **statementId is now server-authoritative** (`InsertRequest.java`,
`InsertStatementResource.java`): `/insert/execute` replaces any client-supplied
statementId with a fresh UUID via `withServerGeneratedStatementId()`.
Idempotent retries correlate via requestId only.
- **Strict ID validation** (`InsertRequest.ID_PATTERN` =
`[A-Za-z0-9_-]{1,128}`): both statementId and requestId are validated on every
construction path (JsonCreator + Builder), and `InsertStatementStore`
re-validates ids before they become znode path components (defense in depth). A
`/` can no longer create nested znodes or collide with another caller's subtree.
- **payloadHash is now computed by the coordinator** from the request it
actually received (`InsertRequest.computePayloadHash()`); a client-supplied
hash is ignored. Reusing a requestId with different rows is now always
detected, including for REST callers that send no hash. The broker no longer
computes/sends a hash at all.
- **Minion task-config allowlist** (`FileInsertExecutor.buildTaskConfigs`):
only `inputFormat`, `includeFileNamePattern`, `excludeFileNamePattern`, and
`recordReader.prop.*` are forwarded from caller options. `input.fs.className`,
`recordReader.className`, `outputDirURI`, `push.*`, `authToken` etc. are
dropped (with a log), closing the scheme-allowlist bypass.
**Correctness**
- **Negative numbers in VALUES** (`InsertIntoValues.java`):
MINUS_PREFIX/PLUS_PREFIX around numeric literals are unwrapped (same treatment
as CalciteSqlParser); tests added. All parse-path errors now throw
`SqlCompilationException` instead of `IllegalArgumentException`.
- **Lead-controller gating**
(`InsertStatementCoordinator.cleanupAllTables`): the sweep now skips tables
this controller does not lead (`LeadControllerManager.isLeaderForTable`), so N
flag-enabled controllers no longer race on the same CAS transitions or all poll
Minion state.
- **Table-drop cleanup**: when the sweep finds a table that no longer
exists, it deletes both `/INSERT_STATEMENTS/{table}` and
`/INSERT_REQUEST_IDS/{table}` subtrees
(`InsertStatementStore.deleteTableSubtrees`), so a recreated table can't serve
the old table's idempotency results.
- **Null partition values** (`ControllerRowInsertExecutor.partitionRows`):
rows with a null partition column are now routed by the column's schema default
null value — the same value the segment creator stores and computes partition
metadata from — preserving the one-partition-per-segment invariant.
- **BYTES / BIG_DECIMAL over the broker wire**
(`SqlQueryExecutor.canonicalizeForWire`): byte[] is sent as HEX string (what
the controller's STRING→BYTES coercion expects) and BigDecimal as a plain
string (no Jackson double truncation), so the broker HTTP path and the
in-process controller path now behave identically. New IT covers negative INT,
BYTES, BIG_DECIMAL, negative DOUBLE end-to-end through the broker.
**Resource limits (ROW inserts build segments on the controller)**
- New configs: `controller.insert.row.max.rows.per.statement` (default 10k),
`controller.insert.row.max.bytes.per.statement` (default 10MB) — both enforced
pre-acceptance, before a manifest is created — and
`controller.insert.row.max.concurrent.statements` (default 4, fail-fast
semaphore in the executor).
- The ROW upload path now runs the same checks as the segment-upload REST
path at staging time: `SegmentValidationUtils.validateTimeInterval`,
`validateUpsertSegmentPartitionMetadata`, and `checkStorageQuota` (wired to the
controller's `StorageQuotaChecker`). An over-quota table can no longer grow
through INSERT. If Helix registration fails after the deep-store copy, the
orphan tar is deleted.
**Docs / cleanup**
- Removed stale comments describing PREPARED/COMMITTED transitions the v1
enum doesn't have; remaining mentions explicitly say "reserved for v2 / not
entered".
- REST API docs updated: idempotency via requestId, hash computed
server-side, statementId server-generated.
**Grammar note (thread on parserImpls.ftl):** `INSERT INTO t` without
FROM/VALUES no longer parses — `DataFileDefList()` is now a required
alternative in the unified `SqlInsertStatement()` production. Previously it
parsed into a `SqlInsertFromFile` with a null file list that failed later at
execution; failing at parse time is intentional. Calling it out here since it's
a (benign) grammar behavior change.
**Not done in this pass (acknowledged):** splitting the coordinator into
ROW-only v1 + FILE/sweep follow-up (thread on the 1.9k-line file), and the
larger IT matrix (REALTIME upload-next-to-consuming, full upsert partition
assignment, multi-controller, hybrid tableType) — proposing to track those as
follow-ups; the riskiest wire-fidelity paths are now covered by the new typed
round-trip 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
