jpisaac commented on PR #2400: URL: https://github.com/apache/phoenix/pull/2400#issuecomment-4422585019
## Code Review: PHOENIX-7795 PR #2400 — Tenant TTL using Tenant Views on Multi-Tenant Table ### Summary Thorough review (two rounds, extreme depth) covering the new validation, `WhereOptimizer` changes, `CompactionScanner` trie behavior, hierarchy constraint interactions, and `PHOENIX_UPDATABLE_VIEW_RESTRICTION_ENABLED` interactions. Happy-path tests are excellent. Three issues found at the edges. --- ### [MEDIUM] `hasTTLProperty` returns `true` for `TTL = NONE` / `TTL = 0` — false-positive rejection on CREATE path **File**: `CreateTableCompiler.java:451-458` `hasTTLProperty` checks for the presence of the `TTL` property key without examining the resolved value. Both `TTL = NONE` and `TTL = 0` resolve to `TTL_EXPRESSION_NOT_DEFINED` via `TTLExpressionFactory` (i.e. semantically "no TTL"), but `hasTTLProperty` returns `true`. This is passed as `newViewHasTTL` to `validateTenantViewWithoutWhereTTLCoexistence`, which rejects the CREATE with `TENANT_VIEW_WITHOUT_WHERE_TTL_CONFLICT`. The ALTER path at `MetaDataClient.java:6628` correctly gates on `!newTTL.equals(TTL_EXPRESSION_NOT_DEFINED)` — the CREATE path should do the same. **Concrete consequence**: `CREATE VIEW v2 AS SELECT * FROM t TTL = NONE` fails when a no-WHERE non-TTL sibling exists, even though the new view has no effective TTL. **Suggested fix**: resolve through `TableProperty.TTL.getValue()` or compute the effective TTL the same way `MetaDataClient.createTableInternal` does, and treat `null` / `TTL_EXPRESSION_NOT_DEFINED` as "no TTL". --- ### [HIGH] Silent fail-open in `WhereOptimizer.getRowKeyMatcher` — compaction-time NPE **File**: `WhereOptimizer.java:504-510` When `ScanUtil.getTenantIdBytes` throws `SQLException` (tenant-id type mismatch), the catch block sets `tenantIdBytes = EMPTY_BYTE_ARRAY`. For a no-WHERE view this causes `getRowKeyMatcher` to return `EMPTY_BYTE_ARRAY`, which is stored as `NULL` for `ROW_KEY_MATCHER` in `SYSTEM.CATALOG`. At compaction time, `CompactionScanner.getTTLInfo` reads `null` for `ROW_KEY_MATCHER` and constructs a `TableTTLInfo` with `matchPattern = null` and a real TTL expression. The gate at `CompactionScanner.java:623` passes (TTL is defined), and `matcher.put(null, tableId)` hits `key.length` on a null key — **NullPointerException** that crashes the region's compaction scanner. The null-matcher path exists pre-PR (WHERE-scoped views where `keySlots == null`), but this PR adds a new, more likely trigger for it. **Suggested fix**: (a) re-throw the `SQLException` for no-WHERE tenant views with TTL so the DDL fails at CREATE time — this is the correct behavior since it tells the user their tenant-id is incompatible before they store data they can't expire; (b) add a null guard before `matcher.put` in `initializeMatcher`/`refreshMatcher` as defense-in-depth for the pre-existing null-matcher path. --- ### [MEDIUM] No-WHERE TTL view shadows WHERE-scoped TTL views in the RowKeyMatcher trie ### [Needs more assessment] **Files**: `RowKeyMatcher.java:95-112`, `CompactionScanner.java:1353-1364` The `RowKeyMatcher` trie returns on shortest prefix match (`get` returns at the first node with a non-null `tableId`). A no-WHERE TTL view's `ROW_KEY_MATCHER` is just the tenant-id prefix (e.g. `\x04acme`), which is strictly shorter than any WHERE-scoped TTL view's matcher (e.g. `\x04acme\x05AUDIT`). When both exist for the same tenant, the no-WHERE prefix shadows the WHERE-scoped entry — all rows get the no-WHERE view's TTL, silently overriding the WHERE-scoped view's TTL. **Example**: ```sql -- Tenant 'acme' creates two sibling views on a multi-tenant table: CREATE VIEW AUDIT_EVENTS AS SELECT * FROM EVENTS WHERE EVENT_TYPE = 'AUDIT' TTL = 200; CREATE VIEW ALL_EVENTS AS SELECT * FROM EVENTS TTL = 100; -- At compaction: audit event rows get TTL=100 instead of TTL=200 ``` **Why existing validations don't prevent this**: 1. **Hierarchy check** (`TTL_ALREADY_DEFINED_IN_HIERARCHY`): checks vertically (ancestors on CREATE, descendants on ALTER), not laterally. Both views are siblings of the base table — neither is an ancestor or descendant of the other. 2. **New coexistence check** (`validateTenantViewWithoutWhereTTLCoexistence`): explicitly excludes WHERE-scoped siblings (`ViewUtil.java:503-505`). By design it only prevents no-WHERE + no-WHERE TTL conflicts. 3. **Updatable view restriction** (`PHOENIX_UPDATABLE_VIEW_RESTRICTION_ENABLED`, PHOENIX-4555): when ON, the outcome is order-dependent. If the WHERE-scoped view is created first (no siblings for check 4) it stays `UPDATABLE` and gets a trie entry — shadowing occurs. If the no-WHERE view is created first, check 4 flips the WHERE-scoped view to `READ_ONLY` (PK column mismatch with the no-WHERE sibling), preventing the trie entry — but then the WHERE-scoped view's TTL is silently non-functional. **Suggested approach**: consider warning or rejecting on CREATE/ALTER of a no-WHERE TTL view when WHERE-scoped TTL siblings already exist for the same tenant, or file a follow-up JIRA to track trie-level multi-TTL resolution (e.g. longest-match-wins). --- ### Missing test coverage - `CREATE VIEW … TTL = NONE` and `… TTL = 0` against a no-WHERE non-TTL sibling — should be allowed. - Tenant has both a no-WHERE TTL view and a WHERE-scoped TTL view — verify which TTL applies to WHERE-matching rows post-compaction. - `WhereOptimizer.getRowKeyMatcher` with `TENANTID_IS_OF_WRONG_TYPE` — verify DDL fails or compaction handles null matcher without NPE. - `ALTER VIEW … SET TTL = NONE` on a tenant view with a sibling — should be allowed (verified clean, worth an explicit test). -- 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]
