[#2553] refactor: refact catalog-jdbc distribution parameter check #2563
Conversation
|
@mchades |
| Index[] indexes) { | ||
| Preconditions.checkArgument( | ||
| null == distribution || distribution == Distributions.NONE, | ||
| "Currently we do not support distribution in Sqlite."); |
There was a problem hiding this comment.
Is it that we don't support distribution or that SQLite doesn't support distribution?
|
|
||
| Preconditions.checkArgument( | ||
| null == distribution || distribution == Distributions.NONE, | ||
| "Currently we do not support distribution in mysql"); |
There was a problem hiding this comment.
Is it that we don't support distribution or that MySQL doesn't support distribution?
There was a problem hiding this comment.
sorry, MySQL, SQlite don't support distribution. The error msg is confusing, I will fix it
There was a problem hiding this comment.
PG also do not support distribution. I will fix all error msg
| @@ -86,6 +86,7 @@ public void testOperationTable() { | |||
| tableComment, | |||
| properties, | |||
| null, | |||
| null, | |||
There was a problem hiding this comment.
Here should use Distributions.NONE instead of null, right? Because here is testing the behaviors after the CatalogOperationDispatcher, so there will be no null here
| Index[] indexes) { | ||
| if (ArrayUtils.isNotEmpty(partitioning)) { | ||
| throw new UnsupportedOperationException( | ||
| "Currently we do not support Partitioning in PostgreSQL"); | ||
| } | ||
| Preconditions.checkArgument( | ||
| null == distribution || distribution == Distributions.NONE, |
There was a problem hiding this comment.
Is it necessary to judge null here?
There was a problem hiding this comment.
I think it's necessray, user use table API may use null for distribution.
There was a problem hiding this comment.
Actually, null has already been transformed into Distributions.NONE before reaching here through CatalogOperationDispatcher:
https://github.com/datastrato/gravitino/blob/0544f2a56896078bb0c97a60e7be2352c036cddc/core/src/main/java/com/datastrato/gravitino/catalog/CatalogOperationDispatcher.java#L475
There was a problem hiding this comment.
Sorry, I did not notice code in CatalogOperationDispatcher. I will remove null judgement here
…n parameter check (apache#2563) ### What changes were proposed in this pull request? Refact catalog-jdbc distribution parameter check, move to catalog-jdbc-mysql 、postgres、sqlite ### Why are the changes needed? Fix: apache#2553 Doris support distribution ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? UT、IT
What changes were proposed in this pull request?
Refact catalog-jdbc distribution parameter check, move to catalog-jdbc-mysql 、postgres、sqlite
Why are the changes needed?
Fix: #2553
Doris support distribution
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
UT、IT