-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[improvement](jdbc catalog) Optimize JdbcCatalog case mapping stability #41510
base: master
Are you sure you want to change the base?
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
run buildall |
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.
Add need to add more FE unit test to test all these logics
String mappedName = nameMapping.getOrDefault(name, name); | ||
String localName = isLowerCaseMetaNames ? mappedName.toLowerCase() : mappedName; | ||
public interface IdentifierMapping { | ||
List<String> fromRemoteDatabaseName(List<String> remoteDatabaseNames); |
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.
Add comment for all these methods
localNameToRemoteName.computeIfAbsent(localName, k -> name); | ||
|
||
if (isLowerCaseMetaNames && !lowerCaseNames.add(localName)) { | ||
if (nameMap.containsKey(localName)) { |
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.
Looks like the nameMap
must contains localName
if goes here?
+ " use meta_name_mapping to specify the column names."); | ||
} | ||
for (int i = 0; i < remoteColumns.size(); i++) { | ||
remoteColumns.get(i).setName(localColumnNames.get(i)); |
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.
Do not modify the input parameter
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 I think, the method fromRemoteColumnName
should be act same as fromRemoteDbName
and fromRemoteTableName
, to return List<String>
instead of List<Column>
.
Which means these method only handle Names
, you can wrap it and generate Column
object outside
return columnMap; | ||
} | ||
|
||
private <K, V> V getRequiredMapping(Map<K, V> map, K key, String typeName, String entityName) { |
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 entityName
seems unnecessary.
private <K, V> V getRequiredMapping(Map<K, V> map, K key, String typeName, String entityName) { | ||
V value = map.get(key); | ||
if (value == null) { | ||
LOG.warn("No remote {} found for {}: {}. Please refresh this catalog.", typeName, typeName, entityName); |
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.
LOG.warn("No remote {} found for {}: {}. Please refresh this catalog.", typeName, typeName, entityName); | |
LOG.warn("No remote {} found for: {}. Please refresh this catalog.", typeName, key); |
} | ||
|
||
@Override | ||
protected void buildDatabaseMapping() { |
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.
this method's logic is same as listDatabaseNames()
, extract one
@@ -149,6 +150,9 @@ public abstract class ExternalCatalog | |||
protected Optional<Boolean> useMetaCache = Optional.empty(); | |||
protected MetaCache<ExternalDatabase<? extends ExternalTable>> metaCache; | |||
|
|||
protected IdentifierMapping identifierMapping; | |||
private boolean mappingsInitialized = false; |
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 using a separator flag mappingsInitialized
?
Why not just using initialized
?
public List<String> fromRemoteTableName(String remoteDbName, List<String> remoteTableNames) { | ||
// If mapping is not required, return the original input | ||
if (!isLowerCaseMetaNames && isMappingInvalid()) { | ||
return remoteTableNames; |
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 user set Config.lower_case_table_names = 1
,
I think we also need to handle the name mapping?
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.
In IdentifierMapping, we only care about the lower_case_meta_names attribute in catalog, not the lower_case_table_names in fe.conf. We put fe.conf in ExternalDatabase for processing.
1f71caf
to
a833914
Compare
run buildall |
a833914
to
a310c8f
Compare
run buildall |
a310c8f
to
1b1af81
Compare
run buildall |
This PR makes the following changes to the uppercase and lowercase mapping of JdbcCatalog