Copilot commented on code in PR #37181: URL: https://github.com/apache/shardingsphere/pull/37181#discussion_r2573307436
########## database/connector/dialect/oracle/src/main/java/org/apache/shardingsphere/database/connector/oracle/resultset/OracleResultSetMapper.java: ########## @@ -0,0 +1,68 @@ +/* + * 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.shardingsphere.database.connector.oracle.resultset; + +import org.apache.shardingsphere.database.connector.core.resultset.DialectResultSetMapper; + +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Types; + +/** + * Result set mapper of Oracle. + */ +public final class OracleResultSetMapper implements DialectResultSetMapper { + + /** + * Oracle JDBC driver type code for TIMESTAMP WITH TIME ZONE. + * Oracle uses non-standard type code -101 instead of {@link Types#TIMESTAMP_WITH_TIMEZONE}. + */ + private static final int ORACLE_TIMESTAMP_WITH_TIME_ZONE = -101; + + /** + * Oracle JDBC driver type code for TIMESTAMP WITH LOCAL TIME ZONE. + */ + private static final int ORACLE_TIMESTAMP_WITH_LOCAL_TIME_ZONE = -102; + + @Override + public Object getSmallintValue(final ResultSet resultSet, final int columnIndex) throws SQLException { + return resultSet.getInt(columnIndex); + } + + @Override + public Object getDateValue(final ResultSet resultSet, final int columnIndex) throws SQLException { + return resultSet.getDate(columnIndex); + } + + @Override + public Object getTimestampValue(final ResultSet resultSet, final int columnIndex, final int columnType) throws SQLException { + if (isOracleTimestampWithTimeZone(columnType)) { + return resultSet.getTimestamp(columnIndex); + } + return resultSet.getObject(columnIndex); + } Review Comment: The method documentation is missing. Since this method is overriding the default implementation and handling Oracle-specific type codes, it should include a JavaDoc comment explaining: - The purpose of this override - Why Oracle timestamp types need special handling - The relationship between Oracle's non-standard type codes and the standard `Types.TIMESTAMP` Example: ```java /** * Get timestamp value from result set. * Handles Oracle-specific timestamp type codes (-101, -102) by calling getTimestamp() * to return a Comparable java.sql.Timestamp instead of Oracle-specific objects. * * @param resultSet result set * @param columnIndex column index * @param columnType column type from metadata * @return timestamp value * @throws SQLException sql exception */ ``` ########## database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/resultset/DialectResultSetMapper.java: ########## @@ -48,4 +48,17 @@ public interface DialectResultSetMapper extends DatabaseTypedSPI { * @throws SQLException sql exception */ Object getDateValue(ResultSet resultSet, int columnIndex) throws SQLException; + + /** + * Get timestamp value from result set. + * + * @param resultSet result set + * @param columnIndex column index + * @param columnType column type from metadata + * @return timestamp value + * @throws SQLException sql exception + */ + default Object getTimestampValue(ResultSet resultSet, int columnIndex, int columnType) throws SQLException { Review Comment: The method name `getTimestampValue()` is misleading. Based on how it's called in `ResultSetMapper.load()`, this method is invoked for ALL unhandled SQL types in the default case (not just timestamp types). Consider renaming it to something more generic like `getDefaultValue()` or `getUnhandledTypeValue()` to accurately reflect its purpose. ```suggestion * Get default value from result set for unhandled SQL types. * * @param resultSet result set * @param columnIndex column index * @param columnType column type from metadata * @return default value * @throws SQLException sql exception */ default Object getDefaultValue(ResultSet resultSet, int columnIndex, int columnType) throws SQLException { ``` ########## database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/resultset/ResultSetMapper.java: ########## @@ -91,7 +91,9 @@ public Object load(final ResultSet resultSet, final int columnIndex) throws SQLE case Types.ARRAY: return resultSet.getArray(columnIndex); default: - return resultSet.getObject(columnIndex); + return dialectResultSetMapper.isPresent() + ? dialectResultSetMapper.get().getTimestampValue(resultSet, columnIndex, metaData.getColumnType(columnIndex)) + : resultSet.getObject(columnIndex); Review Comment: The new behavior where the default case delegates to `dialectResultSetMapper.getTimestampValue()` lacks test coverage. The existing test `assertLoadObjectValue()` in `ResultSetMapperTest` should be updated to verify both code paths: 1. When a `DialectResultSetMapper` is present (should call `getTimestampValue()`) 2. When no `DialectResultSetMapper` is present (should call `resultSet.getObject()`) This is important to ensure the fallback behavior works correctly. ########## database/connector/core/src/main/java/org/apache/shardingsphere/database/connector/core/resultset/ResultSetMapper.java: ########## @@ -91,7 +91,9 @@ public Object load(final ResultSet resultSet, final int columnIndex) throws SQLE case Types.ARRAY: return resultSet.getArray(columnIndex); default: - return resultSet.getObject(columnIndex); + return dialectResultSetMapper.isPresent() + ? dialectResultSetMapper.get().getTimestampValue(resultSet, columnIndex, metaData.getColumnType(columnIndex)) + : resultSet.getObject(columnIndex); } } Review Comment: The default case now always delegates to `getTimestampValue()` for ALL unhandled types, not just timestamp-related types. This is semantically incorrect as the default case handles many different SQL types (e.g., `Types.JAVA_OBJECT`, `Types.REF`, `Types.STRUCT`, etc.). Consider checking if the column type is a timestamp variant before delegating to `getTimestampValue()`. For example: ```java default: if (dialectResultSetMapper.isPresent() && isTimestampType(metaData.getColumnType(columnIndex))) { return dialectResultSetMapper.get().getTimestampValue(resultSet, columnIndex, metaData.getColumnType(columnIndex)); } return resultSet.getObject(columnIndex); ``` Or rename the method to something more generic like `getDefaultValue()` to better reflect its purpose of handling all default cases. ```suggestion int sqlType = metaData.getColumnType(columnIndex); if (dialectResultSetMapper.isPresent() && isTimestampType(sqlType)) { return dialectResultSetMapper.get().getTimestampValue(resultSet, columnIndex, sqlType); } return resultSet.getObject(columnIndex); } } /** * Check if the SQL type is a timestamp variant. * * @param sqlType SQL type from java.sql.Types * @return true if timestamp type, false otherwise */ private static boolean isTimestampType(final int sqlType) { return sqlType == Types.TIMESTAMP || sqlType == Types.TIMESTAMP_WITH_TIMEZONE; } ``` -- 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]
