Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the
📝 WalkthroughWalkthroughAdds three SAPIQ Rosetta extractor classes: a new Columns extractor with type mapping and nullable parsing, plus simple Tables and View extractors. Also updates README translation matrix to include an SAP IQ (Sybase) column. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@source/src/main/java/com/adataptivescale/rosetta/source/core/extractors/column/SAPIQColumnsExtractor.java`:
- Around line 42-44: The code uses
String.valueOf(resultSet.getString("TYPE_NAME")) which turns SQL NULL into the
literal "null" and then feeds that into TranslationMatrix; instead, retrieve the
raw String via resultSet.getString("TYPE_NAME"), check for null, and only call
TranslationMatrix.getInstance().findBySourceTypeAndSourceColumnType("sapiq",
columnType) and setTypeName when columnType != null; apply the same change to
the other extractor classes (BigQueryColumnsExtractor, RedshiftColumnsExtractor,
PostgresColumnsExtractor, SnowflakeColumnsExtractor, SpannerColumnsExtractor,
MySQLColumnsExtractor, KineticaColumnsExtractor) replacing the
String.valueOf(...) usage and guarding the TranslationMatrix lookup with a null
check.
...in/java/com/adataptivescale/rosetta/source/core/extractors/column/SAPIQColumnsExtractor.java
Outdated
Show resolved
Hide resolved
|
Make sure you also udpate the README.md and also include the new Driver in the Driver Download process |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
82-83:⚠️ Potential issue | 🔴 CriticalAdd SAP IQ driver to the driver download process.
While SAP IQ is implemented in the codebase and appears in the README support matrix, it is missing from the downloadable drivers list. The
cli/src/main/resources/drivers.yamlfile contains 10 drivers but does not include SAP IQ, and consequently,docs/markdowns/download_drivers.mdalso lacks SAP IQ documentation. Users attempting to userosetta initor therosetta driverscommand cannot access the SAP IQ driver.Add SAP IQ (Sybase) driver entry to
drivers.yamland updatedownload_drivers.mdwith the corresponding download link and JDBC connection string example.
🤖 Fix all issues with AI agents
In `@README.md`:
- Around line 31-43: The README matrix is inaccurate because SAP IQ extractors
(SAPIQColumnsExtractor, SAPIQTablesExtractor, SAPIQViewExtractor) call
TranslationMatrix but translation.csv lacks SAP IQ type mappings so translations
fail; add SAP IQ entries mapping native SAP IQ types to each target (BigQuery,
Snowflake, MySQL, Postgres, Kinetica, Spanner, SQL Server, DB2, Oracle,
Redshift) in translator/src/main/resources/translation_matrix/translation.csv,
add any SAP IQ-specific attributes to translation_attribute.csv, update the
README table entries for SAP IQ (and Kinetica if mappings are missing) to
reflect true support, and add tests that exercise SAPIQ* extractors and
TranslationMatrix mapping (asserting translated target types) to validate the
fix.
🧹 Nitpick comments (1)
README.md (1)
86-106: Consider adding a SAP IQ connection configuration example.Since this PR introduces SAP IQ support, it would be helpful to include a SAP IQ connection example in the configuration section. This would guide users on the correct JDBC connection string format and required parameters for SAP IQ.
📝 Suggested SAP IQ configuration example
Add the following example to demonstrate SAP IQ connectivity:
userName: postgres password: sakila + - name: sapiq + databaseName: your_database + schemaName: DBA + dbType: sapiq + url: jdbc:sybase:Tds:localhost:2638?ServiceName=your_database + userName: DBA + password: your_passwordNote: The exact JDBC URL format should match the SAP IQ jConnect driver requirements. Please verify the correct URL pattern, default port (typically 2638 for SAP IQ), and required connection parameters.
Summary by CodeRabbit