Copilot commented on code in PR #63783:
URL: https://github.com/apache/doris/pull/63783#discussion_r3315231042
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/client/JdbcConnectorClient.java:
##########
@@ -242,6 +250,15 @@ private void initializeDataSource(String url, String user,
String password,
}
}
+ private void configureSnowflakeOauth(HikariDataSource dataSource, String
url, String password) {
+ if (dbType != JdbcDbType.SNOWFLAKE || password == null ||
password.isEmpty() || url == null) {
+ return;
+ }
+ if (url.toLowerCase(Locale.ROOT).contains("authenticator=oauth")) {
+ dataSource.addDataSourceProperty("token", password);
+ }
+ }
Review Comment:
Using `contains("authenticator=oauth")` can mis-detect non-OAuth
authenticator values (e.g., `authenticator=oauth2`) and incorrectly set the
`token` property. Prefer parsing the query string and requiring
`authenticator`’s value to equal `oauth` (case-insensitive) to avoid false
positives.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/client/JdbcSnowflakeConnectorClient.java:
##########
@@ -0,0 +1,174 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.connector.jdbc.client;
+
+import org.apache.doris.connector.api.ConnectorType;
+import org.apache.doris.connector.jdbc.JdbcDbType;
+
+import java.sql.Types;
+import java.util.HashSet;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+
+/**
+ * Snowflake-specific JDBC connector client.
+ */
+public class JdbcSnowflakeConnectorClient extends JdbcConnectorClient {
+
+ public JdbcSnowflakeConnectorClient(
+ String catalogName, JdbcDbType dbType, String jdbcUrl,
+ boolean onlySpecifiedDatabase,
+ Map<String, Boolean> includeDatabaseMap,
+ Map<String, Boolean> excludeDatabaseMap,
+ boolean enableMappingVarbinary,
+ boolean enableMappingTimestampTz) {
+ super(catalogName, dbType, jdbcUrl, onlySpecifiedDatabase,
+ includeDatabaseMap, excludeDatabaseMap,
+ enableMappingVarbinary, enableMappingTimestampTz);
+ }
+
+ @Override
+ protected Set<String> getFilterInternalDatabases() {
+ Set<String> set = new HashSet<>();
+ set.add("information_schema");
+ return set;
+ }
+
+ @Override
+ public ConnectorType jdbcTypeToConnectorType(JdbcFieldInfo fieldInfo) {
+ String typeName =
fieldInfo.getDataTypeName().orElse("").toUpperCase(Locale.ROOT);
+ switch (typeName) {
+ case "BOOLEAN":
+ return ConnectorType.of("BOOLEAN");
+ case "BYTEINT":
+ case "TINYINT":
+ return ConnectorType.of("TINYINT");
+ case "SMALLINT":
+ return ConnectorType.of("SMALLINT");
+ case "INTEGER":
+ case "INT":
+ return ConnectorType.of("INT");
+ case "BIGINT":
+ return ConnectorType.of("BIGINT");
+ case "NUMBER":
+ case "DECIMAL":
+ case "NUMERIC":
+ case "FIXED":
+ return createDecimalOrString(fieldInfo.requiredColumnSize(),
fieldInfo.requiredDecimalDigits());
+ case "REAL":
+ case "FLOAT":
+ case "FLOAT4":
+ case "FLOAT8":
+ case "DOUBLE":
+ case "DOUBLE PRECISION":
+ return ConnectorType.of("DOUBLE");
+ case "DATE":
+ return ConnectorType.of("DATEV2");
+ case "TIME":
+ return ConnectorType.of("STRING");
+ case "TIMESTAMP":
+ case "DATETIME":
+ case "TIMESTAMP_NTZ":
+ return ConnectorType.of("DATETIMEV2",
timestampScale(fieldInfo), -1);
+ case "TIMESTAMP_LTZ":
+ case "TIMESTAMP_TZ":
+ if (enableMappingTimestampTz) {
+ return ConnectorType.of("TIMESTAMPTZ",
timestampScale(fieldInfo), -1);
+ }
+ return ConnectorType.of("DATETIMEV2",
timestampScale(fieldInfo), -1);
+ case "CHAR":
+ case "CHARACTER":
+ case "VARCHAR":
+ case "TEXT":
+ case "STRING":
+ return ConnectorType.of("STRING");
+ case "BINARY":
+ case "VARBINARY":
+ return enableMappingVarbinary
+ ? ConnectorType.of("VARBINARY",
fieldInfo.requiredColumnSize(), -1)
+ : ConnectorType.of("STRING");
+ case "VARIANT":
+ case "OBJECT":
+ case "ARRAY":
+ case "GEOGRAPHY":
+ case "GEOMETRY":
+ case "VECTOR":
+ return ConnectorType.of("STRING");
+ default:
+ return fallbackByJdbcType(fieldInfo);
+ }
+ }
+
+ private ConnectorType fallbackByJdbcType(JdbcFieldInfo fieldInfo) {
+ switch (fieldInfo.getDataType()) {
+ case Types.BOOLEAN:
+ case Types.BIT:
+ return ConnectorType.of("BOOLEAN");
+ case Types.TINYINT:
+ return ConnectorType.of("TINYINT");
+ case Types.SMALLINT:
+ return ConnectorType.of("SMALLINT");
+ case Types.INTEGER:
+ return ConnectorType.of("INT");
+ case Types.BIGINT:
+ return ConnectorType.of("BIGINT");
+ case Types.FLOAT:
+ case Types.REAL:
+ return ConnectorType.of("FLOAT");
+ case Types.DOUBLE:
+ return ConnectorType.of("DOUBLE");
Review Comment:
The fallback mapping returns `FLOAT` for `Types.FLOAT/REAL`, but the
type-name-based mapping treats Snowflake `FLOAT` as `DOUBLE`. This can lead to
inconsistent inferred schemas depending on whether `TYPE_NAME` is present.
Align fallback behavior with the primary mapping (e.g., map `Types.FLOAT/REAL`
to `DOUBLE` for Snowflake) to avoid surprising type changes.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProvider.java:
##########
@@ -111,6 +117,63 @@ public void validateProperties(Map<String, String>
properties) {
}
}
+ private static void validateSnowflakeProperties(Map<String, String>
properties) {
+ String jdbcUrl = resolve(properties, JdbcConnectorProperties.JDBC_URL);
+ if (jdbcUrl == null ||
!jdbcUrl.toLowerCase().startsWith("jdbc:snowflake:")) {
+ return;
+ }
Review Comment:
`toLowerCase()` here is locale-sensitive and can behave incorrectly under
certain default locales (e.g., Turkish). Use `toLowerCase(Locale.ROOT)` (or
equivalent) for protocol/prefix comparisons.
##########
fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java:
##########
@@ -323,13 +329,40 @@ private TTableDescriptor
buildTestTableDescriptor(ConnectorValidationContext con
private TOdbcTableType parseOdbcType() {
String jdbcUrl =
properties.getOrDefault(JdbcConnectorProperties.JDBC_URL, "");
JdbcDbType dbType = JdbcDbType.parseFromUrl(jdbcUrl);
+ return toOdbcTableType(dbType);
+ }
+
+ static TOdbcTableType toOdbcTableType(JdbcDbType dbType) {
try {
return TOdbcTableType.valueOf(dbType.name());
} catch (Exception e) {
return TOdbcTableType.MYSQL;
}
}
+ static String applySnowflakeConnectionProperties(String jdbcUrl,
Map<String, String> properties) {
+ String database =
properties.get(JdbcConnectorProperties.SNOWFLAKE_DATABASE);
+ String warehouse =
properties.get(JdbcConnectorProperties.SNOWFLAKE_WAREHOUSE);
+ if (database == null || database.trim().isEmpty()
+ || warehouse == null || warehouse.trim().isEmpty()) {
+ return jdbcUrl;
+ }
+ String separator = jdbcUrl.contains("?") ? "&" : "?";
+ if (jdbcUrl.endsWith("?") || jdbcUrl.endsWith("&")) {
+ separator = "";
+ }
+ return jdbcUrl + separator + "db=" +
encodeQueryParameter(database.trim())
+ + "&warehouse=" + encodeQueryParameter(warehouse.trim());
+ }
+
+ private static String encodeQueryParameter(String value) {
+ try {
+ return URLEncoder.encode(value, "UTF-8");
+ } catch (UnsupportedEncodingException e) {
+ return value;
+ }
+ }
Review Comment:
Prefer `URLEncoder.encode(value, StandardCharsets.UTF_8)` to avoid a checked
exception that can’t occur for UTF-8 in modern JVMs and reduce boilerplate.
Also consider whether `URLEncoder`’s `+` encoding for spaces is acceptable for
your JDBC URL usage; if not, use a URI/query builder that encodes spaces as
`%20`.
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/SnowflakeJdbcAuthUtils.java:
##########
@@ -0,0 +1,49 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.jdbc;
+
+import com.zaxxer.hikari.HikariDataSource;
+
+import java.util.Locale;
+
+/**
+ * Snowflake JDBC accepts OAuth credentials through the "token" connection
+ * property. Doris stores the secret in jdbc_password so SHOW CREATE CATALOG
+ * keeps masking it.
+ */
+public final class SnowflakeJdbcAuthUtils {
+
+ private SnowflakeJdbcAuthUtils() {
+ }
+
+ public static void configure(HikariDataSource dataSource, String jdbcUrl,
String jdbcPassword) {
+ if (dataSource == null || jdbcPassword == null ||
jdbcPassword.isEmpty()
+ || !isSnowflakeOauthUrl(jdbcUrl)) {
+ return;
+ }
+ dataSource.addDataSourceProperty("token", jdbcPassword);
+ }
+
+ static boolean isSnowflakeOauthUrl(String jdbcUrl) {
+ if (jdbcUrl == null) {
+ return false;
+ }
+ String lowerUrl = jdbcUrl.toLowerCase(Locale.ROOT);
+ return lowerUrl.startsWith("jdbc:snowflake:") &&
lowerUrl.contains("authenticator=oauth");
Review Comment:
Same false-positive risk as in FE: `contains("authenticator=oauth")` will
match values like `authenticator=oauth2`. Parse the query parameters and check
that `authenticator` exists and exactly equals `oauth` (case-insensitive).
##########
fe/be-java-extensions/jdbc-scanner/src/main/java/org/apache/doris/jdbc/JdbcTypeHandlerFactory.java:
##########
@@ -54,6 +54,8 @@ public static JdbcTypeHandler create(String tableType) {
return new TrinoTypeHandler();
case "GBASE":
return new GbaseTypeHandler();
+ case "SNOWFLAKE":
+ return new DefaultTypeHandler();
default:
return new DefaultTypeHandler();
Review Comment:
The explicit `SNOWFLAKE` branch currently returns the same handler as
`default`, which is redundant and can suggest special behavior that isn’t
actually present. Either remove the branch (and rely on `default`) or introduce
a dedicated handler if Snowflake needs distinct behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
