-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Enhancement] Unified Catalog support Paimon #47398
Conversation
} | ||
return properties; | ||
} | ||
|
||
@Override | ||
public List<String> getPartitionColumnNames() { | ||
return partColumnNames; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The most risky bug in this code is:
The getProperties()
method may return a mutable reference to the internal properties
map, potentially leading to unintended modifications from outside the class.
You can modify the code like this:
@Override
public Map<String, String> getProperties() {
if (properties == null) {
this.properties = new HashMap<>();
}
return new HashMap<>(properties); // Return a defensive copy of the properties map
}
96da9ff
to
0348d6b
Compare
DELTALAKE, new DeltaLakeConnector(derivedContext), | ||
KUDU, new KuduConnector(derivedContext) | ||
); | ||
boolean shouldCreatePaimonConnector = null != context.getProperties().get(PAIMON_CATALOG_WAREHOUSE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why you check this config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paimon has multiple checks in its SDK. In the real usage of our customers, it may come to many validation problems when getting metadata in unified catalog even if there is no Paimon table in the catalog. So I add a judgement to filter out Paimon if its warehouse configure does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is necessary, we can move this line to a place just before line 77.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and contains
would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments, overall LGTM.
DELTALAKE, new DeltaLakeConnector(derivedContext), | ||
KUDU, new KuduConnector(derivedContext) | ||
); | ||
boolean shouldCreatePaimonConnector = null != context.getProperties().get(PAIMON_CATALOG_WAREHOUSE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is necessary, we can move this line to a place just before line 77.
DELTALAKE, new DeltaLakeConnector(derivedContext), | ||
KUDU, new KuduConnector(derivedContext) | ||
); | ||
boolean shouldCreatePaimonConnector = null != context.getProperties().get(PAIMON_CATALOG_WAREHOUSE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and contains
would be better.
Quality Gate passedIssues Measures |
.put(HUDI, new HudiConnector(derivedContext)) | ||
.put(DELTALAKE, new DeltaLakeConnector(derivedContext)) | ||
.put(KUDU, new KuduConnector(derivedContext)); | ||
boolean containsPaimon = null != context.getProperties().get(PAIMON_CATALOG_WAREHOUSE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you move this check to CatalogAnalyzer
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems hard cause it's for unified catalog only. Any ideas?
please rebase main |
Signed-off-by: Jiao Mingye <[email protected]>
Signed-off-by: Jiao Mingye <[email protected]>
7154bd0
to
0ff519f
Compare
Quality Gate passedIssues Measures |
[FE Incremental Coverage Report]✅ pass : 17 / 20 (85.00%) file detail
|
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
@Mergifyio backport branch-3.3 |
@Mergifyio backport branch-3.2 |
✅ Backports have been created
|
✅ Backports have been created
|
## Why I'm doing: Make unified catalog support paimon data sources. example: ```sql CREATE EXTERNAL CATALOG unified PROPERTIES ( "type" = "unified", "unified.metastore.type" = "hive", "hive.metastore.uris" = "thrift://localhost:9083", "paimon.catalog.warehouse" = "hdfs://127.0.0.1:9000/paimon", ); ``` Signed-off-by: Jiao Mingye <[email protected]> (cherry picked from commit 2e50513) # Conflicts: # fe/fe-core/src/test/java/com/starrocks/connector/unified/UnifiedMetadataTest.java
## Why I'm doing: Make unified catalog support paimon data sources. example: ```sql CREATE EXTERNAL CATALOG unified PROPERTIES ( "type" = "unified", "unified.metastore.type" = "hive", "hive.metastore.uris" = "thrift://localhost:9083", "paimon.catalog.warehouse" = "hdfs://127.0.0.1:9000/paimon", ); ``` Signed-off-by: Jiao Mingye <[email protected]> (cherry picked from commit 2e50513) # Conflicts: # fe/fe-core/src/main/java/com/starrocks/catalog/PaimonTable.java # fe/fe-core/src/main/java/com/starrocks/connector/unified/UnifiedConnector.java # fe/fe-core/src/main/java/com/starrocks/connector/unified/UnifiedMetadata.java # fe/fe-core/src/test/java/com/starrocks/connector/unified/UnifiedConnectorTest.java # fe/fe-core/src/test/java/com/starrocks/connector/unified/UnifiedMetadataTest.java
Make unified catalog support paimon data sources. example: ```sql CREATE EXTERNAL CATALOG unified PROPERTIES ( "type" = "unified", "unified.metastore.type" = "hive", "hive.metastore.uris" = "thrift://localhost:9083", "paimon.catalog.warehouse" = "hdfs://127.0.0.1:9000/paimon", ); ``` Signed-off-by: Jiao Mingye <[email protected]> (cherry picked from commit 2e50513)
Make unified catalog support paimon data sources. example: ```sql CREATE EXTERNAL CATALOG unified PROPERTIES ( "type" = "unified", "unified.metastore.type" = "hive", "hive.metastore.uris" = "thrift://localhost:9083", "paimon.catalog.warehouse" = "hdfs://127.0.0.1:9000/paimon", ); ``` Signed-off-by: Jiao Mingye <[email protected]> (cherry picked from commit 2e50513)
Why I'm doing:
Make unified catalog support paimon data sources.
example:
What I'm doing:
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: