palashc commented on PR #2400: URL: https://github.com/apache/phoenix/pull/2400#issuecomment-4424633584
## Changes Summary — PR Review Round 2 ### Issue 1: `hasTTLProperty` false positive **File**: `CreateTableCompiler.java` - `hasTTLProperty` now resolves the TTL value through `TableProperty.TTL.getValue()` and returns `false` when it resolves to `TTL_EXPRESSION_NOT_DEFINED` (i.e., `TTL = NONE` or `TTL = 0`). - Added imports for `TableProperty` and `LiteralTTLExpression.TTL_EXPRESSION_NOT_DEFINED`. ### Issue 2: NPE risk from empty `ROW_KEY_MATCHER` on TTL views **CREATE-path fail-fast** — `CreateTableCompiler.java`: - If `where == null && newViewHasTTL && rowKeyMatcher.length == 0 && parent.isMultiTenant() && tenantId != null` → throw `TENANTID_IS_OF_WRONG_TYPE`. - Preserves existing `TenantIdTypeIT` behavior (no-TTL views with inconvertible tenant-id still succeed). **Defense-in-depth** — `CompactionScanner.java`: - `initializeMatcher` and `refreshMatcher` now skip `TableTTLInfo` entries whose `matchPattern` is `null` or empty (log warn instead of inserting a null key into the trie). ### Tests added — `TenantTTLIT.java` - `testCreateNoWhereViewWithTTLNoneAllowedWithSibling` — `CREATE VIEW ... TTL = 'NONE'` and `... TTL = 0` allowed alongside a no-WHERE non-TTL sibling. - `testAlterTTLToNoneAllowedWithSibling` — `ALTER VIEW ... SET TTL = 'NONE'` allowed even with siblings. - `testCreateNoWhereTTLViewFailsForInconvertibleTenantId` — `CREATE VIEW ... TTL = 10` with inconvertible tenant-id fails fast with `TENANTID_IS_OF_WRONG_TYPE`; no-TTL view in same scenario still succeeds. ### Issue 3 - Skipped per prior agreement with reviewer. ### Verification | Suite | Tests | Result | |---|---|---| | `TenantTTLIT` | 16 | Pass | | `TenantIdTypeIT` | 15 | Pass (no regression) | | `ViewTTLIT` | 68 | Pass (no regression) | FYI @jpisaac -- 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]
