Cdap e2e framework update#667
Conversation
…snapshot bump snapshot
…_version_fix cdap e2e framework snapshot version fix
Redshift source and connector plugin added.
…tion [PLUGIN-1708] Redshift source and connector plugin added.
Redshift Junits
…ases [Plugin-1708] Redshift Junits
…n-args [PLUGIN-1728]Fix for connection arguments not getting updated.
…ucumber-report-url Add cucumber report url to the workflow
Fix non-nullable datetime when zeroDateTimeBehavior is CONVERT_TO_NULL.
…ssue [PLUGIN-1793] Oracle sink plugin failing with null error when input schema contains small case letters.
Add e2e test case for small case schema.
…est-case [PLUGIN-1793]Add e2e test case for small case schema.
e2e workflow added for release branches
…uln-fix-develop [Security Vulnerability] Run build with unit tests without elevated permissions
…ections Enable TLS Support (DTS - Specific)
Added fix for date datatype
…ssue-dev [PLUGIN-1812] Added fix for date datatype in oracle sink
…column-names-cloud-mysql [PLUGIN-1813] CloudSQLMySQL Escape column names
…4650e13274f73e8c173c2895170f512ef4245ee [🍒][Plugin-1779] Add TRANSACTION_ISOLATION_LEVEL config in MySQL, PostgreSQL & SQL Server plugins
…00f23f51a8e6834e8e0fd680c0c6effed8ba9ed [🍒][PLUGIN-1815] Commit/Rollback added in Committer.
…napshot/rc-1-12-2 Remove Snapshot for release 1.12.2 database plugins
… like a flag for Backward compatibility issues for Timestamp and number (precisionless)
…y_PLUGIN-1893_6_11 [cherry-pick][6.11][PLUGIN-1893] Adding fields in Oracle source and connector which acts like a flag for Backward compatibility issues for Timestamp and number (precisionless)
…ing_snap_for_hub_rel removing Snapshot for hub release
…ss_cdf_611 CDAP OSS migration for 6.11
…e-test-deps [🍒] Remove test deps
…aven-publishing-cherrypick [🍒] Add required for maven central publishing
…hot-remove Removing snapshot for CDAP 6.11.1 release
…ypick/6552c2878531670975e0ffc2a1dbc95f9ac4ec47 [🍒] Verify Goal Removal
…tampLTZ-change [cherrypick] [PLUGIN-1932] Add hidden treatTimestampLTZAsTimestamp flag to map Oracle TIMESTAMP WITH LOCAL TIME ZONE to TIMESTAMP
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request introduces significant updates across multiple database plugins, including the addition of transaction isolation level support, enhanced error reporting with external documentation links, and the implementation of upsert operations for Oracle. It also refactors the core database commons to improve connection management through task-level commit logic. Review feedback highlights critical issues, including a reference to an undefined variable in the Oracle plugin, potential SQL syntax errors when constructing upsert queries, and incorrect argument ordering in error message formatting.
| if (!errorMessage.endsWith(".")) { | ||
| errorMessage = errorMessage + "."; | ||
| } | ||
| errorMessage = String.format("%s For more details, see %s", errorMessageWithDetails, errorMessage); |
There was a problem hiding this comment.
The externalDocumentationLink is not included in the formatted error message, even though it is checked for presence on line 322. Additionally, the arguments in String.format appear to be incorrect (swapped) compared to the implementation in AbstractDBSource, which will lead to a confusing error summary.
| errorMessage = String.format("%s For more details, see %s", errorMessageWithDetails, errorMessage); | |
| errorMessage = String.format("%s For more details, see %s", errorMessage, externalDocumentationLink); |
| boolean firstUpdateColumn = true; | ||
| for (String fieldName : fieldNames) { | ||
| boolean isKeyColumn = false; | ||
| for (String listKey : listKeys) { | ||
| String listKeyNoQuote = listKey.replace("\"", ""); | ||
| if (listKeyNoQuote.equals(fieldName)) { | ||
| isKeyColumn = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!isKeyColumn) { | ||
| if (!firstUpdateColumn) { | ||
| query.append(", "); | ||
| } | ||
| query.append("target.").append(fieldName).append(" = source.").append(fieldName); | ||
| firstUpdateColumn = false; | ||
| } | ||
| } |
There was a problem hiding this comment.
If all columns in fieldNames are also present in listKeys, the UPDATE SET clause will be empty, which results in invalid Oracle SQL syntax for the MERGE statement. The WHEN MATCHED clause should only be appended if there are non-key columns to update. Additionally, the nested loop for key lookup is inefficient ($O(N \times M)$); using a Set for lookups is recommended.
| boolean firstUpdateColumn = true; | |
| for (String fieldName : fieldNames) { | |
| boolean isKeyColumn = false; | |
| for (String listKey : listKeys) { | |
| String listKeyNoQuote = listKey.replace("\"", ""); | |
| if (listKeyNoQuote.equals(fieldName)) { | |
| isKeyColumn = true; | |
| break; | |
| } | |
| } | |
| if (!isKeyColumn) { | |
| if (!firstUpdateColumn) { | |
| query.append(", "); | |
| } | |
| query.append("target.").append(fieldName).append(" = source.").append(fieldName); | |
| firstUpdateColumn = false; | |
| } | |
| } | |
| StringBuilder updateClause = new StringBuilder(); | |
| boolean firstUpdateColumn = true; | |
| java.util.Set<String> keySet = new java.util.HashSet<>(); | |
| for (String listKey : listKeys) { | |
| keySet.add(listKey.replace("\"", "")); | |
| } | |
| for (String fieldName : fieldNames) { | |
| if (!keySet.contains(fieldName)) { | |
| if (!firstUpdateColumn) { | |
| updateClause.append(", "); | |
| } | |
| updateClause.append("target.").append(fieldName).append(" = source.").append(fieldName); | |
| firstUpdateColumn = false; | |
| } | |
| } | |
| if (!firstUpdateColumn) { | |
| query.append("WHEN MATCHED THEN UPDATE SET ").append(updateClause).append(" "); | |
| } |
| String fieldName, int fieldIndex) throws SQLException { | ||
| int sqlType = columnTypes.get(fieldIndex).getType(); | ||
| int sqlIndex = fieldIndex + 1; | ||
| int sqlType = modifiableColumnTypes.get(fieldIndex).getType(); |
There was a problem hiding this comment.
The variable modifiableColumnTypes is used but does not appear to be defined in this class or its parent. It should likely be columnTypes, which is the field used in the previous version of the code and in OracleSinkDBRecord. If a modifiable version was intended, ensure it is properly declared and initialized.
| int sqlType = modifiableColumnTypes.get(fieldIndex).getType(); | |
| int sqlType = columnTypes.get(fieldIndex).getType(); |
No description provided.