Copilot commented on code in PR #64740:
URL: https://github.com/apache/doris/pull/64740#discussion_r3458729298
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcClient.java:
##########
@@ -426,22 +427,23 @@ public List<Column> getColumnsFromJdbc(String
remoteDbName, String remoteTableNa
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
Review Comment:
The comment claims `DatabaseMetaData.getPrimaryKeys` orders rows by
`COLUMN_NAME`, but per the JDBC spec the result set is expected to be ordered
by `KEY_SEQ` (drivers may violate this). Consider updating the comment to
reflect that this is a driver-behavior workaround (e.g., 'some drivers (e.g.,
MySQL) return rows in COLUMN_NAME order; enforce KEY_SEQ ordering here').
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcClient.java:
##########
@@ -426,22 +427,23 @@ public List<Column> getColumnsFromJdbc(String
remoteDbName, String remoteTableNa
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
Review Comment:
PR description says only MySQL behavior is corrected, but this change
updates the base `JdbcClient.getPrimaryKeys` for all JDBC sources. Either
adjust the PR description to reflect the broader behavior change, or scope the
`KEY_SEQ` reordering to the MySQL-specific implementation if that was the
intent.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcMySQLClient.java:
##########
@@ -202,21 +203,22 @@ public List<JdbcFieldSchema> getJdbcColumnsInfo(String
remoteDbName, String remo
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
Review Comment:
Using a `TreeMap<Short, String>` will silently overwrite entries if the
driver returns duplicate `KEY_SEQ` values, potentially dropping PK columns. A
safer approach is to collect rows into a list of `(keySeq, columnName)` pairs
and sort by `keySeq`, or map `KEY_SEQ` to a list.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcMySQLClient.java:
##########
@@ -202,21 +203,22 @@ public List<JdbcFieldSchema> getJdbcColumnsInfo(String
remoteDbName, String remo
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
try {
DatabaseMetaData databaseMetaData = conn.getMetaData();
rs = databaseMetaData.getPrimaryKeys(remoteDbName, null,
remoteTableName);
while (rs.next()) {
- String fieldName = rs.getString("COLUMN_NAME");
- primaryKeys.add(fieldName);
+ primaryKeys.put(rs.getShort("KEY_SEQ"),
rs.getString("COLUMN_NAME"));
Review Comment:
Using a `TreeMap<Short, String>` will silently overwrite entries if the
driver returns duplicate `KEY_SEQ` values, potentially dropping PK columns. A
safer approach is to collect rows into a list of `(keySeq, columnName)` pairs
and sort by `keySeq`, or map `KEY_SEQ` to a list.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcMySQLClient.java:
##########
@@ -202,21 +203,22 @@ public List<JdbcFieldSchema> getJdbcColumnsInfo(String
remoteDbName, String remo
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
try {
DatabaseMetaData databaseMetaData = conn.getMetaData();
rs = databaseMetaData.getPrimaryKeys(remoteDbName, null,
remoteTableName);
while (rs.next()) {
- String fieldName = rs.getString("COLUMN_NAME");
- primaryKeys.add(fieldName);
+ primaryKeys.put(rs.getShort("KEY_SEQ"),
rs.getString("COLUMN_NAME"));
}
} catch (SQLException e) {
throw new JdbcClientException("failed to get jdbc primary key info
for remote table `%s.%s`: %s",
remoteDbName, remoteTableName,
Util.getRootCauseMessage(e));
} finally {
close(rs, conn);
}
- return primaryKeys;
+ return Lists.newArrayList(primaryKeys.values());
Review Comment:
Using a `TreeMap<Short, String>` will silently overwrite entries if the
driver returns duplicate `KEY_SEQ` values, potentially dropping PK columns. A
safer approach is to collect rows into a list of `(keySeq, columnName)` pairs
and sort by `keySeq`, or map `KEY_SEQ` to a list.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/jdbc/client/JdbcMySQLClient.java:
##########
@@ -202,21 +203,22 @@ public List<JdbcFieldSchema> getJdbcColumnsInfo(String
remoteDbName, String remo
public List<String> getPrimaryKeys(String remoteDbName, String
remoteTableName) {
Connection conn = getConnection();
ResultSet rs = null;
- List<String> primaryKeys = Lists.newArrayList();
+ // getPrimaryKeys orders rows by COLUMN_NAME, not KEY_SEQ; reorder by
the 1-based KEY_SEQ
+ // to keep the real composite-PK column order.
+ TreeMap<Short, String> primaryKeys = new TreeMap<>();
Review Comment:
The `KEY_SEQ` reordering logic is now duplicated in both `JdbcClient` and
`JdbcMySQLClient`. Consider extracting a small shared helper (e.g., a method
that consumes the `ResultSet` and returns ordered column names) to keep
behavior consistent and reduce future drift.
--
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]
