PHOENIX-7753 : Allow uncovered index creation on tables with relaxed conditional TTL #2357
Conversation
phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
Show resolved
Hide resolved
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1, do we already have test where creation of uncovered index fails on strict conditional TTL? if not, let's add such test
phoenix-core-client/src/main/java/org/apache/phoenix/schema/MetaDataClient.java
Show resolved
Hide resolved
| boolean isStrictTTL = | ||
| metaProperties.isStrictTTL() != null ? metaProperties.isStrictTTL : table.isStrictTTL(); | ||
| if (!(newTTL instanceof ConditionalTTLExpression) || isStrictTTL) { | ||
| newTTL.validateTTLOnAlter(connection, table); |
There was a problem hiding this comment.
It would be better if you pass the isStrictTTL to the validateTTLOnAlter function and then in the specific implementation of conditional ttl expression skip the checks for uncovered index if the ttl is not strict.
@virajjasani Yes we do. https://github.com/apache/phoenix/blob/master/phoenix-core/src/test/java/org/apache/phoenix/schema/ConditionalTTLExpressionTest.java#L538 |
| @@ -2529,7 +2530,14 @@ private PTable createTableInternal(CreateTableStatement statement, byte[][] spli | |||
| } else { | |||
| ttlFromHierarchy = checkAndGetTTLFromHierarchy(parent, tableName); | |||
There was a problem hiding this comment.
@palashc I think it would be better if we can enhance the API checkAndGetTTLFromHierarchy itself to return TTL_EXPRESSION_NOT_DEFINED in this case.
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Can you also add a test case where the table has uncovered index and a literal TTL. Now you alter the ttl to conditional with IS_STRICT_TTL set to false. Then verify that the table has conditional ttl and the uncovered index has ttl undefined. I am suspecting that while you skipped checking for uncovered indexes during the ttl validation on alter, I am not seeing that you are actually updating the ttl on uncovered index to undefined.
There was a problem hiding this comment.
@tkhurana should we update the uncovered index ttl from the client or should we handle it on the server side when building the ptable for the uncovered index?
There was a problem hiding this comment.
I think for CDC index we handle it when building the PTable. You can do something similar.
tkhurana
left a comment
tkhurana
left a comment
There was a problem hiding this comment.
Left some more comments
3 participants
No description provided.