Feature/sqlserver support #32
Conversation
Codecov Report
@@ Coverage Diff @@
## master #32 +/- ##
==========================================
+ Coverage 88.93% 89.62% +0.69%
==========================================
Files 7 7
Lines 235 241 +6
Branches 31 25 -6
==========================================
+ Hits 209 216 +7
+ Misses 26 25 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Apologies for the long time to review. I don't look very ofter for the PRs here. (feel free to ping me, send e-mail, etc to raise attention).
though I'm not a Scala expert
No issues with your scala code.
PR overall looks good, still I left some comments more on how to introduce and maintain this feature.
| } else { | ||
| "no_table_name" | ||
| } | ||
| val normalizedTableName = normalizeForAvro(tableName) |
| @@ -124,7 +124,7 @@ object JdbcAvroJob { | |||
| val generatedSchema: Schema = createSchema(p, args) | |||
| saveString(subPath(output, "/_AVRO_SCHEMA.avsc"), generatedSchema.toString(true)) | |||
|
|
|||
| val queries: Iterable[String] = args.buildQueries() | |||
| val queries: Iterable[String] = args.buildQueries(args.connectionUrl) | |||
There was a problem hiding this comment.
Instead of passing args.connectionUrl and checking if isSqlServer() in many places, I think we can introduce something like:
class DefaultQueryBuilder() {
def buildQueries(..): Iterable[String] = // here old implementation
def buildSingleRorQuery(tableName: String): String = s"SELECT * FROM $tableName LIMIT 1"
}
class SqlServerQueryBuilder() extends DefaultQueryBuilder {
def buildQueries(..): Iterable[String] = // here sql server implementation
def buildSingleRorQuery(tableName: String): String = s"SELECT TOP 1 * FROM $tableName"
}
| @@ -116,6 +116,7 @@ lazy val dbeamCore = project | |||
| "org.apache.commons" % "commons-dbcp2" % "2.1.1", | |||
| "org.postgresql" % "postgresql" % "42.2.+", | |||
| "mysql" % "mysql-connector-java" % "5.1.+", | |||
| "com.microsoft.sqlserver" % "mssql-jdbc" % "6.4.0.jre8", | |||
There was a problem hiding this comment.
I would prefer not to add mssql-jdbc jar as a dependency to dbeam-core. MySQL and PostgreSQL are defined as dependencies because of the GA support from Google Cloud SQL.
That being said, I don't think it is a problem to add some code to handle sqlserver. I suggest as an alternative to create a dbeam-mssql project/artifact on build.sbt which would depend on mssql-jdbc.
I tried to minimize the impact of the changes as much as possible, though I'm not a Scala expert. Please provide feedback and let's iterate over the proposes changes. Adding support for SQL Server is a requirement of high interest for my client and the beginning of other contributions we intend to make.