Provide Struct data type support for oracle plugin#659
Provide Struct data type support for oracle plugin#659vanshikaagupta22 wants to merge 1 commit intodata-integrations:developfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for resolving Oracle STRUCT types into CDAP RECORD schemas by querying the ALL_TYPE_ATTRS metadata table. The implementation includes recursive resolution for nested structures and an updated mapping for primitive Oracle types. Feedback highlights several critical issues: the metadata query lacks an OWNER filter which could lead to incorrect schema resolution in multi-schema environments; fully qualified type names containing dots will cause IllegalArgumentException when creating CDAP records; and there are logic errors in the primitive type mapping, specifically regarding Oracle-specific type naming conventions and an invalid comparison between Java class names and SQL type strings.
| String sql = "SELECT * FROM ALL_TYPE_ATTRS WHERE TYPE_NAME = ? ORDER BY ATTR_NO"; | ||
|
|
||
| try (PreparedStatement stmt = connection.prepareStatement(sql)) { | ||
| stmt.setString(1, typeName.substring(typeName.lastIndexOf('.') + 1)); |
There was a problem hiding this comment.
The query to ALL_TYPE_ATTRS should include a filter on the OWNER column. Without it, if multiple schemas define a type with the same name, the query will return attributes for all of them, leading to an incorrect or corrupted schema.
Additionally, the typeName provided by the JDBC driver is often fully qualified (e.g., OWNER.TYPE_NAME). You should parse the owner from the typeName string if present, and fallback to the provided schemaName otherwise.
| if (attrSchema != null) { | ||
| fields.add(Schema.Field.of(attrName, attrSchema)); | ||
| } else { | ||
| Schema nestedSchema = getStructSchema(connection, schemaName, attrTypeName); |
There was a problem hiding this comment.
When recursing for nested STRUCT types, the schemaName should be updated to the owner of the attribute's type to ensure the subsequent query filters correctly. This owner can be retrieved from the ATTR_TYPE_OWNER column in the ALL_TYPE_ATTRS result set.
| schemaName, typeName)); | ||
| } | ||
|
|
||
| return Schema.recordOf(typeName, fields); |
There was a problem hiding this comment.
CDAP Schema.recordOf requires the record name to be a valid identifier (matching [A-Za-z_][A-Za-z0-9_]*). If typeName is fully qualified (e.g., MY_SCHEMA.MY_TYPE), this call will throw an IllegalArgumentException due to the dot. Consider using only the simple type name part for the record name.
String recordName = typeName.contains(".") ? typeName.substring(typeName.lastIndexOf('.') + 1) : typeName;
return Schema.recordOf(recordName, fields);| case "TIMESTAMP WITH TZ": | ||
| return isTimestampOldBehavior ? Schema.of(Schema.Type.STRING) : Schema.of(Schema.LogicalType.TIMESTAMP_MICROS); | ||
| case "TIMESTAMP WITH LTZ": | ||
| return getTimestampLtzSchema(); | ||
| case "TIMESTAMP": | ||
| return Schema.of(Schema.LogicalType.DATETIME); | ||
| case "DATE" : | ||
| return Schema.of(Schema.LogicalType.DATE); | ||
| case "BINARY FLOAT": | ||
| case "FLOAT": | ||
| return Schema.of(Schema.Type.FLOAT); | ||
| case "BINARY DOUBLE": | ||
| case "DOUBLE": | ||
| return Schema.of(Schema.Type.DOUBLE); | ||
| case "BFILE": | ||
| case "RAW": | ||
| case "LONG RAW": | ||
| return Schema.of(Schema.Type.BYTES); | ||
| case "INTERVAL DAY TO SECOND": | ||
| case "INTERVAL YEAR TO MONTH": | ||
| case "VARCHAR2": | ||
| case "VARCHAR": | ||
| case "CHAR": | ||
| case "CLOB": | ||
| case "BLOB": | ||
| case "LONG": | ||
| return Schema.of(Schema.Type.STRING); | ||
| case "INTEGER": | ||
| return Schema.of(Schema.Type.INT); | ||
| case "NUMBER": | ||
| case "DECIMAL": | ||
| // FLOAT and REAL are returned as java.sql.Types.NUMERIC but with value that is a java.lang.Double | ||
| if (Double.class.getTypeName().equals(typeName)) { | ||
| return Schema.of(Schema.Type.DOUBLE); |
There was a problem hiding this comment.
There are two issues in mapPrimitiveOracleType:
- Type Name Mismatches: The strings used in the
switch(e.g.,"TIMESTAMP WITH TZ","BINARY FLOAT") likely do not match the values returned by Oracle inALL_TYPE_ATTRS.ATTR_TYPE_NAME. For example, Oracle typically uses"TIMESTAMP WITH TIME ZONE","BINARY_FLOAT", and"BINARY_DOUBLE". Please verify these against the Oracle metadata documentation. - Logic Error: At line 243,
Double.class.getTypeName().equals(typeName)compares the Java class name string ("java.lang.Double") with the SQL type name (e.g.,"NUMBER"), which will always be false. Sincemetadatais not available here to check the column class, this check should probably be removed or replaced with a check on the SQL type name if applicable.
vanshikaagupta22
force-pushed
the
oracle-structsupport
branch
from
a88c6b7 to
231c6ee
Compare
1 participant
No description provided.