Allow to create before_ prefixed columns in Consensus Commit
#844
Conversation
| return !metadata.getPrimaryKeyColumnNames().contains(columnName) | ||
| && metadata.getAfterImageColumnNames().contains(columnName); |
There was a problem hiding this comment.
When the column is not primary key column and is an after image column, update the before column of the column.
With this change, users can create before_ prefixed columns as normal columns. If the column name is before_x, the before column name is before_before_x.
There was a problem hiding this comment.
Why don't you use just com.scalar.db.transaction.consensuscommit.TransactionTableMetadata#getBeforeImageColumnNames() ?
(added)^ Sorry, I meant return !metadata.getBeforeImageColumnNames().contains(columnName).
There was a problem hiding this comment.
But return !metadata.getBeforeImageColumnNames().contains(columnName) contains the primary key columns? Or we can use return !metadata.getPrimaryKeyColumnNames().contains(columnName) && !metadata.getBeforeImageColumnNames().contains(columnName), but it looks like the current one is more straightforward. What do you think?
There was a problem hiding this comment.
Oops, got it. I had a wrong idea, and cluster key has before column while partition key doesn't have. Please ignore the previous comment.
| return !metadata.getPrimaryKeyColumnNames().contains(columnName) | ||
| && metadata.getAfterImageColumnNames().contains(columnName); |
There was a problem hiding this comment.
Why don't you use just com.scalar.db.transaction.consensuscommit.TransactionTableMetadata#getBeforeImageColumnNames() ?
(added)^ Sorry, I meant return !metadata.getBeforeImageColumnNames().contains(columnName).
| @@ -70,6 +78,10 @@ public Set<String> getSecondaryIndexNames() { | |||
| return tableMetadata.getSecondaryIndexNames(); | |||
| } | |||
|
|
|||
| public LinkedHashSet<String> getPrimaryKeyColumnNames() { | |||
There was a problem hiding this comment.
This is a very off-topic question and just out of curiosity, but why LinkedHashSet is used not just more abstract Set? I've not looked over all the usages. But it doesn't seem to need to be the concrete class type (sorry if I'm missing something)
There was a problem hiding this comment.
LinkedHashSet contains the context of the fixed iteration order (the insertion order). But Set doesn't have any context of the iteration order. We wanted to add the context of the iteration order to the return type.
There was a problem hiding this comment.
I see. So, List would be a simpler? It has contains() and ImmutableLinkedHashSet's contains() seems O(n) as well as List's one
There was a problem hiding this comment.
Yeah, using List is another possible idea. But the difference is that Set doesn't allow to have the same element, but List does. But, if I re-design this class, I would use List. Thanks.
feeblefakie
deleted the
allow-to-create-before-prefixed-columns-in-consensus-commit
branch
Currently, when we create a non-primary key column with a name that starts with
before_, the column is not handled correctly. Specifically, the before column is not updated correctly in the prepare phase, as explained in the following comment:#841 (comment)
This PR fixes the issue. Please take a look!