FANNG1 commented on PR #11715: URL: https://github.com/apache/gravitino/pull/11715#issuecomment-4729957458
Thanks for tackling the Doris 3.0/4.0 compatibility work — this is valuable. One process suggestion: I think this PR is worth splitting. It currently bundles ~6–7 independent concerns (+1185/−84). They're cleanly decoupled, and mixing the low-risk changes (type/enum mapping, removing a validation) with the one stateful, higher-risk piece (the `SHOW CREATE TABLE` cache + generated-column parsing) means the whole PR is gated on its riskiest part and is hard to review thoroughly in a single pass. Suggested split, ordered by dependency/risk: 1. **Type system** — `DorisTypeConverter`: `datev2` + new type mappings + `datetime(N)` precision. Pure functions, easy to review/merge first. 2. **Index syntax** — `DorisTableOperations`: `USING` clause, `UNIQUE KEY` model, `mapDorisIndexType`, ADD/DROP back-quoting. Independent, low risk. 3. **Partition compatibility** — `DorisUtils`: multi-line / space-tolerant regex + LIST value assignments. Self-contained. 4. **Complex / generated column loading** — `DorisTableOperations`: `getColumnBuilder` override + `SHOW CREATE TABLE` cache + generated-column extraction. This is the only part that introduces cross-call state, so isolating it lets reviewers focus on cache lifecycle and parsing edge cases, and lets it be reverted independently. (Depends on #1 for type parsing.) 5. **Auto-increment enablement** — tiny; standalone or folded into #2. Dependencies: #4 depends on #1; the other three are independent and can proceed in parallel. I also did a review pass, and the correctness issues I found cluster exactly in #3 and #4 — for example, the generated-column splitter increments angle-bracket depth on every `<` (so a `<` inside an `AS (...)` expression breaks column splitting and drops later generated columns), and the `SHOW CREATE TABLE` cache is never invalidated, so `loadTable` after an `ALTER` can return stale column types. That clustering is the main reason I'd isolate those two. Happy to post the detailed findings as inline comments if helpful. If a single PR is preferred to keep the end-to-end IT story together, then at minimum splitting out #4 (the stateful piece) would help a lot; otherwise per-concern commits would already make review much easier. -- 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]
