-
Notifications
You must be signed in to change notification settings - Fork 12
Avatical Removal Part1: remove Avatica dependency from metadata conversion flow #136
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
base: main
Are you sure you want to change the base?
Conversation
4bd32a0 to
40ea34f
Compare
jdbc-util/src/main/java/com/salesforce/datacloud/jdbc/metadata/SqlType.java
Outdated
Show resolved
Hide resolved
| immutableEntry("int4", JDBCType.INTEGER.toString()), | ||
| immutableEntry("oid", JDBCType.BIGINT.toString()), | ||
| immutableEntry("int8", JDBCType.BIGINT.toString()), | ||
| immutableEntry("float", JDBCType.DOUBLE.toString()), |
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.
@mkaufmann confirm whether FLOAT --> double conversion is correct?
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.
It's pretty unclear where the names here are coming from as they don't seem to be SQL types. The only use case of this map seems to be for private static List<Object> constructColumnData(ResultSet resultSet) throws SQLException {
That method doesn't really make sense though as it seems to be only used for convering a ResultSet in the end to a result set. So I think this + constructColumnData can just be completely removed. Instead of all that complication instead this function
public static ResultSet createColumnResultSet(
String schemaPattern, String tableNamePattern, String columnNamePattern, Connection connection)
throws SQLException {
final List<Object> data;
try (val statement = connection.createStatement()) {
val getColumnsQuery = getColumnsQueryInner(schemaPattern, tableNamePattern, columnNamePattern);
val resultSet = statement.executeQuery(getColumnsQuery);
data = constructColumnData(resultSet);
}
return getMetadataResultSet(QueryDBMetadata.GET_COLUMNS, data);
}
Should be adjusted so that it directly return the resultSet from statement.executeQuery(getColumnsQuery); and to enable that the query needs to be adjusted to directly return the columns with the expected names and types for GET_COLUMNS. That can be achieved by using CAST( ... AS <TYPE>) and as <COLUMN_NAME> in the top level select
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.
@mkaufmann i think we can remove some amount of the code through refactoring, but the base problem still exists (i.e. we still need to determine the correct type mapping).
The names which we're seeing in the above map (such as int2, float4...etc) are base hyper types (we see the same in Postgres) that are fetched by querying the internal db table pg_catalog.pg_attribute for column metadata. Agreed, they're not SQL types, but they are the way column types are stored in this internal db table, and so we need to have a mapping between such base db types and SQL types.
If we do away with the mapping, when running metadata.getColumns() the customer will see non-SQL types such as int4 which would break the JDBC contract
282e5d0 to
d56d810
Compare
4716e63 to
8c07f84
Compare
8c07f84 to
bbc22d8
Compare
36255fe to
023405d
Compare
2d45c7c to
c28d02a
Compare
This change accomplishes Part 1 of removing Avatica from the datacloud-jdbc driver - specifically focusing on marshalling of column metadata. It removes Avatica dependencies from metadata conversion and introduces a custom SimpleResultSet implementation for database metadata queries which includes much of the JDBc boilerplate that was originally handled by Avatica.
QueryMetadataUtil now uses SimpleMetadataResultSet.of() instead of Avatica-based constructors, and type mappings were updated to use standard JDBCType instead of Avatica's SqlType enum.