[Destination MSSQLv2] Implement typed insertions #50434
Conversation
This reverts commit 90aa35e.
Co-authored-by: Jonathan Pearlin <jpearlin1@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Co-authored-by: Jonathan Pearlin <jpearlin1@gmail.com>
| } | ||
| } else { | ||
| try { | ||
| when (value) { |
There was a problem hiding this comment.
It may make this cleaner to encapsulate this logic in an extension function on the value -- something like value.setStatementValue(statement=statement). The extension function would encapsulate this when block, making this code easier to test separate from the type conversion expectations.
| .filter { field -> field.name !in airbyteFields } | ||
| .map { field -> | ||
| try { | ||
| when (field.type.type) { |
There was a problem hiding this comment.
Same comment as above...this feels like another good use of an extension function on the type to encapsulate this conversion logic and make it easier to test.
| private val dataSourceFactory: MSSQLDataSourceFactory | ||
| ) : DestinationWriter { | ||
| override fun createStreamLoader(stream: DestinationStream): StreamLoader { | ||
| val dataSource = dataSourceFactory.getDataSource(config) |
There was a problem hiding this comment.
We may want to do this in the constructor/init/post construct method. Otherwise, we are creating a data source per stream, which means d x s connections instead of just d connections if the data source is shared between streams (preferred to limit connections).
| } | ||
| } | ||
| } catch (ex: Exception) { | ||
| KotlinLogging.logger {}.error(ex) { ex.message } |
There was a problem hiding this comment.
This should probably be a file level variable to avoid creating the logger each time that the code catches.
...2/src/test-integration/kotlin/io/airbyte/integrations/destination/mssql/v2/MSSQLCheckTest.kt
Outdated
Show resolved
Hide resolved
| is BooleanValue -> setAsBooleanValue(idx, value) | ||
| is DateValue -> setAsDateValue(idx, value) | ||
| is IntegerValue -> setAsIntegerValue(idx, value) | ||
| NullValue -> setAsNullValue(idx, field.type.type) |
There was a problem hiding this comment.
Is this missing an is?
| is BooleanValue -> "boolean" | ||
| is DateValue -> "string" | ||
| is IntegerValue -> "integer" | ||
| NullValue -> "null" |
There was a problem hiding this comment.
Same comment...is this missing a leading is?
| when (field.type.type) { | ||
| is StringType -> getStringValue(field.name) | ||
| is ArrayType -> getArrayValue(field.name) | ||
| ArrayTypeWithoutSchema -> getArrayValue(field.name) |
There was a problem hiding this comment.
More mixes of is type and just type. If both work, we should be consistent here and choose one approach.
There was a problem hiding this comment.
This is because we are mixing two concepts, AirbyteValue is a sealed interface and we are mixing data object and value class. is refers to value class, cases where is is omitted are data object.
That's also how intellij auto generates the support all cases for a sealed interface, at least with the default style config.
One question/comment on the inconsistent use of the 069a0bc to
b564021
Compare
Co-authored-by: Jonathan Pearlin <jonathan@airbyte.io>
Co-authored-by: Jonathan Pearlin <jonathan@airbyte.io>
What
Handle typed insertion of data.
Can this PR be safely reverted and rolled back?