Provide Struct data type support for oracle plugin - #659
vanshikaagupta22 wants to merge 10 commits into
Conversation
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.
a88c6b7 to
231c6ee
Compare
e6834e3 to
fe03c7b
Compare
# Conflicts: # oracle-plugin/src/main/java/io/cdap/plugin/oracle/OracleSourceSchemaReader.java # oracle-plugin/src/test/java/io/cdap/plugin/oracle/OracleSchemaReaderTest.java
a11f5cc to
af67d36
Compare
af67d36 to
3bfd8ba
Compare
f4ab512 to
47834bb
Compare
47834bb to
219b85e
Compare
| recordBuilder.set(field.getName(), bigDecimal.longValue()); | ||
| break; | ||
| case STRING: | ||
| recordBuilder.set(field.getName(), bigDecimal.toString()); |
3455570 to
7495c92
Compare
046c0be to
d24127c
Compare
Sunish-Dahiya
left a comment
There was a problem hiding this comment.
- Can you please add the testing report.
- UTs are not following standard convention can you please fix that.
- Please also attach coverage details/report.
- I think we would also need to add new ITNs to cover this logic.
- Since this change will require new permission, have we closed the discussion with CDF team ?
| } | ||
| } | ||
|
|
||
| private void handleOffsetDateTimeValue(OffsetDateTime offsetDateTime, Schema fieldSchema, |
There was a problem hiding this comment.
These types would be at top level/root level also and their handling should already exist, any specific reason we are handling record members separately?
There was a problem hiding this comment.
this comment applies to all types handling
There was a problem hiding this comment.
Top-level columns are read directly from the ResultSet using column positions, but STRUCT members come from struct.getAttributes() as raw Java objects where no ResultSet is available. Also, Oracle returns all STRUCT numbers as BigDecimal, dates as Timestamp/OffsetDateTime, and LOBs as Clob/Blob objects. We need this handling in populateRecordField to convert these raw objects into the correct types so that their value can be stored properly.
There was a problem hiding this comment.
May be then we can move these in a separate Struct specific class
d24127c to
ca030ef
Compare
ca030ef to
3a6cb47
Compare
|
Sunish-Dahiya
left a comment
There was a problem hiding this comment.
The test report says coverage is around 30%, why it's so low?
| } | ||
| } | ||
|
|
||
| private void handleOffsetDateTimeValue(OffsetDateTime offsetDateTime, Schema fieldSchema, |
There was a problem hiding this comment.
May be then we can move these in a separate Struct specific class
… into oracle-structsupport
490f15c to
fd4635e
Compare
| handleDecimalValue((BigDecimal) attrValue, fieldSchema, recordBuilder, field); | ||
| return; | ||
| } | ||
| if (attrValue instanceof Timestamp) { |
There was a problem hiding this comment.
AI suggestion:
Struct.getAttributes() in the Oracle JDBC driver returns raw oracle.sql.* datum objects rather than java.time.OffsetDateTime or java.sql.Timestamp:
TIMESTAMP WITH TIME ZONE / TIMESTAMP WITH LOCAL TIME ZONE returns oracle.sql.TIMESTAMPTZ / oracle.sql.TIMESTAMPLTZ (which do not implement OffsetDateTime). Please extract the OffsetDateTime using reflection (offsetDateTimeValue(Connection)) as done in OracleSourceDBRecord.handleTimestampTZ.
TIMESTAMP returns oracle.sql.TIMESTAMP (which does not extend java.sql.Timestamp unless J2EE13Compliant=true).
BINARY_FLOAT / BINARY_DOUBLE can return oracle.sql.BINARY_FLOAT / oracle.sql.BINARY_DOUBLE, and UROWID returns oracle.sql.ROWID (which implements java.sql.RowId, not String).
Without converting these oracle.sql.* types first, they fall through to sourceRecord.populateRecordField and fail with UnexpectedFormatException at runtime.
can you please validate if its true?
| private static void handleTimestampValue(OracleSourceDBRecord sourceRecord, Timestamp timestamp, Schema fieldSchema, | ||
| StructuredRecord.Builder recordBuilder, Schema.Field field, | ||
| Connection connection) throws SQLException { | ||
| if (Schema.LogicalType.DATETIME.equals(fieldSchema.getLogicalType())) { |
There was a problem hiding this comment.
AI Suggestion: In Oracle, DATE attributes inside a STRUCT are returned as Timestamp. If a user overrides the nested field schema to Schema.LogicalType.DATE or Schema.Type.STRING, falling back to sourceRecord.populateRecordField will call recordBuilder.setTimestamp(...) unconditionally and fail with UnexpectedFormatException.
Please validate once
| recordBuilder.setTimestamp(field.getName(), zonedDateTime); | ||
| } else if (Schema.LogicalType.DATETIME.equals(fieldSchema.getLogicalType())) { | ||
| LocalDateTime systemLocalDateTime = offsetDateTime.atZoneSameInstant( | ||
| ZoneId.systemDefault()).toLocalDateTime(); |
There was a problem hiding this comment.
Using ZoneId.systemDefault() shifts TIMESTAMPLTZ values to the worker JVM's OS timezone rather than preserving the database/session local time (compare with OracleSourceDBRecord line 346 which uses resultSet.getTimestamp(columnIndex).toLocalDateTime()). Please avoid ZoneId.systemDefault() so output does not vary by worker machine timezone.
| if (columnTypeName != null && columnTypeName.contains(".")) { | ||
| return columnTypeName.substring(0, columnTypeName.lastIndexOf('.')); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
If typeName is unqualified (e.g., "ADDRESS_TYPE" without "OWNER."), extractOwnerName returns null, and WHERE TYPE_NAME = ? AND OWNER = ? binds NULL to OWNER = ?. In Oracle SQL, OWNER = NULL always returns 0 rows, causing schema resolution to fail with No attributes found
| return null; | ||
| } | ||
|
|
||
| public Schema getSchemaMapping(ColumnMetadata metadata) throws SQLException { |
any update on this? |
This change adds native support for resolving Oracle STRUCT types (Object Types) into CDAP RECORD schemas. By querying the ALL_TYPE_ATTRS metadata table, the schema builder dynamically processes complex structures with support for up to 4 levels of nesting. Internal attributes are first translated to standard SQL data types via a dedicated mapper before being converted into the final CDAP schema.
When reading the data, the implementation overrides the setField() function, providing a custom implementation to properly extract and map individual custom object attributes according to requirements.
Note: Users will require explicit EXECUTE privileges on the custom Oracle object types to successfully resolve the schema.
Test Coverage :
