jiangxt2 commented on PR #11731:
URL: https://github.com/apache/gravitino/pull/11731#issuecomment-4828803045

   Thanks for the review @zhoukangcn! Two responses:
   
   **1. PR scope**
   
   I understand the concern about size. The actual breakdown for Doris-related 
files (13 files, +1267/-81):
   
   - Production code: 1 file (`DorisTableOperations.java`), ~250 lines
   - Unit tests: 3 files, ~335 lines (new 
`TestDorisTableOperationsSqlGeneration`)
   - Integration tests: 3 files, ~590 lines (`CatalogDoris3xIT` / 
`CatalogDoris4xIT`)
   - Test infra (Docker/Container): 6 files, ~170 lines
   
   ~81% of the diff is test code. The INDEX syntax and Auto Increment logic are 
tightly coupled — Auto Increment requires a `UNIQUE KEY` table model 
declaration, and they share `appendTableModelKeySql()` / `mapDorisIndexType()`. 
Splitting them would require reviewers to cross-reference two PRs to understand 
the key-model flow.
   
   That said, if you still prefer a split, I can extract the Docker infra 
changes (DorisImageName, ContainerSuite, docker-compose) into a separate 
prerequisite PR. Let me know your preference.
   
   **2. Backward compatibility**
   
   Valid concern. I verified the behavior on Doris 3.0.6.2 and 4.0.6:
   
   | Syntax | 3.0.6.2 | 4.0.6 |
   |--------|---------|-------|
   | `INDEX name (col)` (bare, no USING) | ✅ defaults to INVERTED | ✅ defaults 
to INVERTED |
   | `INDEX name (col) USING BITMAP` | ✅ accepted, stored as INVERTED | ❌ parse 
error |
   | `INDEX name (col) USING INVERTED` | ✅ | ✅ |
   
   Since bare `INDEX name (col)` defaults to BITMAP on 1.2.x and INVERTED on 
3.0+/4.0+, the simplest backward-compatible fix is to **omit the USING clause 
entirely** for BITMAP indexes — letting each Doris version apply its own 
default. This preserves old behavior on 1.2.x without requiring version 
detection.
   
   I'll update `mapIndexTypeToUsingClause()` to return an empty string for the 
BITMAP case, and adjust the read path (`mapDorisIndexType`) accordingly. Will 
push the fix once the scope question above is settled.


-- 
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]

Reply via email to