🤖 If ddlExecutor.close() throws, control jumps to the outer catch (Exception e) { log.error(...) } (which doesn't rethrow), so partitionClientPool.close() never runs and the Thrift sockets + worker threads leak silently. Could you wrap the pool cleanup in a try-finally so it runs regardless of whether ddlExecutor.close() succeeds? E.g. try { ddlExecutor.close(); } finally { if (partitionClientPool != null) { ... } }.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 The shared SessionState pattern here makes me a bit nervous — every worker thread calls SessionState.start(sharedSessionState) and then runs concurrent Driver.run() against the same instance. Hive's SessionState has plenty of mutable fields the Driver writes during query execution (e.g. currentDatabase via USE, hiveOperation, cmd, lineageState, tablesAlreadyMarkedAsRead). Are we confident none of these get written concurrently by parallel ALTER PARTITION / TOUCH statements? @yihua could you weigh in on whether sharing SessionState across worker threads is safe in practice, or whether each worker should construct its own (paying the SessionState init cost in exchange for isolation)?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 Sharing one SessionState across worker threads is risky here. Hive stores the active SessionState in a ThreadLocal (SessionState.start() sets tss.set(...)), so this pattern installs the same mutable instance into N threads' ThreadLocals. The Hive Driver isn't documented as thread-safe, and Driver execution mutates session-scoped state — currentDatabase (USE), conf overrides, and the compile/execute lineage/read-entity sets — so concurrent ALTER PARTITION/TOUCH runs can interleave on those fields. Worth confirming whether any of the batched statements can issue a USE or set conf; if so, per-worker SessionState (or serializing the session-mutating bits) is the safer default even at the init cost.

🤖 When a Future fails, f.get() throws ExecutionException wrapping the real cause. firstError then stores the ExecutionException, which gets rethrown and caught by the outer catch (Exception e) in addPartitionsToTable/dropPartitionsToTable/registerAlterPartitionEvent, which wraps it again in HoodieHiveSyncException. The result is HoodieHiveSyncException -> ExecutionException -> RealCause — one more wrapping layer than the sequential path produced. HiveDriverPool.awaitAll already has an unwrap() helper; could we do the same here for consistency?

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: the comment says this field is 'kept for reference only — close() is delegated through ddlExecutor.close()', which means it's never read after being passed into HiveQueryDDLExecutor. Could you make it a local variable in the constructor instead? A field that isn't read after construction will make future readers wonder what lifecycle role it plays.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: the parallel-path error-collection loop inside runDropBatches (Exception firstError = null; for (Future<Void> f : futures) …) is nearly identical to the one in HMSDDLExecutor.runBatches. Since HiveDriverPool already has a well-factored awaitAll(List<Future<?>>) for exactly this pattern, it might be worth adding a similar awaitAll(List<Future<Void>>) to IMetaStoreClientPool so both callers can delegate to it instead.

_{- AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 These two pools are built eagerly here, but if maybeBuildPartitionClientPool(...) or the HiveQueryDDLExecutor constructor on the next line throws, the outer catch (line ~148) just rethrows — and since the constructor failed, close() is never called, so the already-built pool's worker threads + Thrift connections leak. Could we close partitionDriverPool/partitionClientPool in the catch before rethrowing? Same pattern in the legacy default-mode branch below.

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: partitionClientPool is a raw nullable while partitionDriverPool just below (line 101) is wrapped in Option<> — both represent the same "batching enabled" condition, so it might be worth making the container type consistent (either both Option<>, or both nullable with the same guard pattern) to avoid a future reader wondering if there's a semantic difference.

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 nit: the doc says "Pool size (number of worker resources) and worker-thread count" — could you simplify to something like "Number of worker threads (and pooled metastore clients) for parallel partition sync when hoodie.datasource.hive_sync.batching.enabled is true"? The current wording lists "pool size" and "worker-thread count" as if they're distinct, but they're the same number.

_{⚠️ AI-generated; verify before applying. React 👍/👎 to flag quality.}

🤖 Sharing one SessionState across worker threads is risky here. Hive stores the active SessionState in a ThreadLocal (SessionState.start() sets tss.set(...)), so this pattern installs the same mutable instance into N threads' ThreadLocals. The Hive Driver isn't documented as thread-safe, and Driver execution mutates session-scoped state — currentDatabase (USE), conf overrides, and the compile/execute lineage/read-entity sets — so concurrent ALTER PARTITION/TOUCH runs can interleave on those fields. Worth confirming whether any of the batched statements can issue a USE or set conf; if so, per-worker SessionState (or serializing the session-mutating bits) is the safer default even at the init cost.