You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Yifan Zhang (Code Review)" <ge...@cloudera.org> on 2024/02/02 11:53:39 UTC
[Impala-ASF-CR] IMPALA-12381: Set JDBC related properties in JDBC data source
Yifan Zhang has posted comments on this change. ( http://gerrit.cloudera.org:8080/20941 )
Change subject: IMPALA-12381: Set JDBC related properties in JDBC data source
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/20941/2/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java
File fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java:
http://gerrit.cloudera.org:8080/#/c/20941/2/fe/src/main/java/org/apache/impala/catalog/DataSourceTable.java@324
PS2, Line 324: if (Strings.isNullOrEmpty(tblInitString)) {
: if (dsPropertyMap != null) {
: // Change keys to lower case.
: for (Map.Entry<String, String> entry : dsPropertyMap.entrySet()) {
: combinedPropertyMap.put(entry.getKey().toLowerCase(), entry.getValue());
: }
: }
: } else {
: // Append additional properties of DataSource to initString.
: try {
: Map<String, String> tblPropertyMap =
: JsonUtil.convertJSonToPropertyMap(tblInitString);
: if (tblPropertyMap != null) {
: // Change keys to lower case.
: for (Map.Entry<String, String> entry : tblPropertyMap.entrySet()) {
: combinedPropertyMap.put(entry.getKey().toLowerCase(), entry.getValue());
: }
: }
: } catch (ImpalaRuntimeException e) {
: // Return initString which is set in the table creation statement if it's
: // invalid JSON string. This could happen for non JDBC data source.
: return tblInitString;
: }
: if (dsPropertyMap != null) {
: for (Map.Entry<String, String> entry : dsPropertyMap.entrySet()) {
: if (!combinedPropertyMap.containsKey(entry.getKey().toLowerCase())) {
: combinedPropertyMap.put(entry.getKey().toLowerCase(), entry.getValue());
: }
: }
: }
: }
Can we reorganize this to:
if (dsPropertyMap != null) {
// Change keys to lower case.
for (Map.Entry<String, String> entry : dsPropertyMap.entrySet()) {
combinedPropertyMap.put(entry.getKey().toLowerCase(), entry.getValue());
}
}
if (Strings.isNullOrEmpty(tblInitString)) {
Map<String, String> tblPropertyMap =
JsonUtil.convertJSonToPropertyMap(tblInitString);
...
}
That would be more readable.
--
To view, visit http://gerrit.cloudera.org:8080/20941
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0a0b5d9d7b06825842828c3722c2bcdb4eacccc8
Gerrit-Change-Number: 20941
Gerrit-PatchSet: 2
Gerrit-Owner: Wenzhe Zhou <wz...@cloudera.com>
Gerrit-Reviewer: Abhishek Rawat <ar...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: gaurav singh <gs...@cloudera.com>
Gerrit-Comment-Date: Fri, 02 Feb 2024 11:53:39 +0000
Gerrit-HasComments: Yes