improvement(catalogs): fix index existence check #7621
Conversation
|
@justinmclean PTAL |
| @@ -774,7 +774,7 @@ static String addIndexDefinition(TableChange.AddIndex addIndex) { | |||
|
|
|||
| static String deleteIndexDefinition( | |||
| JdbcTable lazyLoadTable, TableChange.DeleteIndex deleteIndex) { | |||
| if (deleteIndex.isIfExists()) { | |||
| if (!deleteIndex.isIfExists()) { | |||
| Preconditions.checkArgument( | |||
| Arrays.stream(lazyLoadTable.index()) | |||
| .anyMatch(index -> index.name().equals(deleteIndex.getName())), | |||
There was a problem hiding this comment.
I'm puzzled that since you use !deleteIndex.isIfExists() in L777, should this place be nonMatch NOT anyMatch here?
There was a problem hiding this comment.
I'm puzzled that since you use
!deleteIndex.isIfExists()in L777, should this place benonMatchNOTanyMatchhere?
Thank you for your review. My understanding is that when deleteIndex.isIfExists() is false, the code block will be entered. Next, check whether the name of any index in lazyLoadTable is the same as deleteIndex.getName(). If it is the same, it will run normally. Otherwise, it will report an Index does not exist exception. I don't know if my understanding is correct ?
There was a problem hiding this comment.
My understanding is that if isIfExists returns true, then it will ignore any warnings about non-existent indexes. If it returns false, then you do want to warn about non-existent indexes.
There was a problem hiding this comment.
The method doesn't have the best name, which can be a little confusing.
### What changes were proposed in this pull request? fix index existence check ### Why are the changes needed? Fix: #7591 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? tests added. Co-authored-by: senlizishi <[email protected]>
### What changes were proposed in this pull request? fix index existence check ### Why are the changes needed? Fix: apache#7591 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? tests added. Co-authored-by: senlizishi <[email protected]>
### What changes were proposed in this pull request? fix index existence check ### Why are the changes needed? Fix: apache#7591 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? tests added. Co-authored-by: senlizishi <[email protected]>
What changes were proposed in this pull request?
fix index existence check
Why are the changes needed?
Fix: #7591
Does this PR introduce any user-facing change?
No
How was this patch tested?
tests added.