Support SQL column with NOT NULL constraint #581
Conversation
Turned out that classes BaseFieldTypeBuilder & BaseTypeBuilder are unrelated.
```
public static final class FieldTypeBuilder<R> extends BaseFieldTypeBuilder<R> {
public static class BaseTypeBuilder<R> {
```
Codecov Report
@@ Coverage Diff @@
## master #581 +/- ##
============================================
- Coverage 91.47% 90.16% -1.32%
- Complexity 243 255 +12
============================================
Files 26 27 +1
Lines 927 966 +39
Branches 67 66 -1
============================================
+ Hits 848 871 +23
- Misses 52 60 +8
- Partials 27 35 +8 |
| import static com.spotify.dbeam.avro.FieldTypeHelper.setIntType; | ||
| import static com.spotify.dbeam.avro.FieldTypeHelper.setLongLogicalType; | ||
| import static com.spotify.dbeam.avro.FieldTypeHelper.setLongType; | ||
| import static com.spotify.dbeam.avro.FieldTypeHelper.setStringType; |
There was a problem hiding this comment.
Please avoid static imports and favor using FieldTypeHelper.setStringType directly.
| String message = | ||
| String.format( | ||
| "Mapping #[%d] SQL [%d][%s] => Avro [%s] failed", | ||
| i, sqlType, getSqlTypeName(sqlType), expectedAvroType); |
There was a problem hiding this comment.
| i, sqlType, getSqlTypeName(sqlType), expectedAvroType); | |
| i, sqlType, JdbcAvroSchema.getSqlTypeName(sqlType), expectedAvroType); |
Avoiding static import.
There was a problem hiding this comment.
BTW, testing all mappings looks nice and uncontroversial. Feel free to submit as a separate PR (easier to review and ship).
| meta.getPrecision(i), | ||
| columnClassName, | ||
| useLogicalTypes, | ||
| isNullTypeSupported, |
There was a problem hiding this comment.
Looks like there is no need to pass isNullTypeSupported boolean around.
if (isNullTypeSupported) {
fieldSchemaBuilder = field.type().unionOf().nullBuilder().endNull().and();
} else {
fieldSchemaBuilder = field.type();
}
// then later
if/else .endUnion().nullDefault();Also, in that case no need for FieldTypeHelper and setAvroColumnType() can stay the way it is.. Doing so, this PR can be simpler.
There was a problem hiding this comment.
I wish it would be that easy.
Unfortunately, in Avro these two similar methods type() belong to unrelated classes
BaseFieldTypeBuilder and BaseTypeBuilder.
| @@ -123,6 +123,9 @@ com.spotify.dbeam.options.JdbcExportPipelineOptions: | |||
| --useAvroLogicalTypes=<Boolean> | |||
| Default: false | |||
| Controls whether generated Avro schema will contain logicalTypes or not. | |||
| --supportAvroNotNullTypes=<Boolean> | |||
| Default: false | |||
| Controls whether generated Avro schema will support not null types. | |||
There was a problem hiding this comment.
Is this a breaking change? i.e. when people simply upgrade DBeam will the auto generated schema that had nullable fields become (possibly) non nullable?
We should avoid such schema auto generation breaking changes..
There was a problem hiding this comment.
Is this a breaking change?
No, by default it has the same behaviour (backwards compatible).
dbeam-core/src/test/java/com/spotify/dbeam/avro/JdbcAvroSchemaTest.java
Outdated
Show resolved
Hide resolved
Creates an Avro schema with a field
typeinstead of[null, type]for SQL column withNOT NULLconstrain.Configurable behaviour.
Checklist for PR author(s)
mvn com.coveo:fmt-maven-plugin:format org.codehaus.mojo:license-maven-plugin:update-file-header)