Support SQL column with NOT NULL constraint #581

rulle-io · 2023-02-11T21:44:59Z

Creates an Avro schema with a field type instead of [null, type] for SQL column with NOT NULL constrain.
Configurable behaviour.

Checklist for PR author(s)

Changes are covered by unit tests (no major decrease in code coverage %) and/or integration tests.
Ensure code formating (use mvn com.coveo:fmt-maven-plugin:format org.codehaus.mojo:license-maven-plugin:update-file-header)
Document any relevant additions/changes in the appropriate spot in javadocs/docs/README.

Turned out that classes BaseFieldTypeBuilder & BaseTypeBuilder are unrelated. ``` public static final class FieldTypeBuilder<R> extends BaseFieldTypeBuilder<R> { public static class BaseTypeBuilder<R> { ```

codecov · 2023-02-11T21:47:02Z

Codecov Report

Merging #581 (de21f4e) into master (1668c7d) will decrease coverage by 1.32%.
The diff coverage is 70.96%.

❗ Current head de21f4e differs from pull request most recent head ef39bed. Consider uploading reports for the commit ef39bed to get more accurate results

@@             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

labianchin · 2023-03-31T16:56:46Z

dbeam-core/src/main/java/com/spotify/dbeam/avro/JdbcAvroSchema.java

+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;


Please avoid static imports and favor using FieldTypeHelper.setStringType directly.

labianchin · 2023-03-31T17:00:23Z

dbeam-core/src/test/java/com/spotify/dbeam/avro/JdbcAvroSchemaTest.java

+      String message =
+          String.format(
+              "Mapping #[%d] SQL [%d][%s] => Avro [%s] failed",
+              i, sqlType, getSqlTypeName(sqlType), expectedAvroType);


Suggested change

i, sqlType, getSqlTypeName(sqlType), expectedAvroType);

i, sqlType, JdbcAvroSchema.getSqlTypeName(sqlType), expectedAvroType);

Avoiding static import.

BTW, testing all mappings looks nice and uncontroversial. Feel free to submit as a separate PR (easier to review and ship).

labianchin · 2023-03-31T17:05:50Z

dbeam-core/src/main/java/com/spotify/dbeam/avro/JdbcAvroSchema.java

+          meta.getPrecision(i),
+          columnClassName,
+          useLogicalTypes,
+          isNullTypeSupported,


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.

I wish it would be that easy.
Unfortunately, in Avro these two similar methods type() belong to unrelated classes
BaseFieldTypeBuilder and BaseTypeBuilder.

labianchin · 2023-03-31T17:09:12Z

README.md

@@ -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.


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..

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

Ruslan Altynnikov added 3 commits January 22, 2023 17:29

First failed attempt to support SQL NOT NULL column/ AVRO field.

0fed6f2

Turned out that classes BaseFieldTypeBuilder & BaseTypeBuilder are unrelated. ``` public static final class FieldTypeBuilder<R> extends BaseFieldTypeBuilder<R> { public static class BaseTypeBuilder<R> { ```

Compilable code.

0f15b63

Unit-tests work.

de21f4e

Ruslan Altynnikov added 2 commits February 14, 2023 00:43

Add more tests.

37feca1

Fix formatting.

bebbc84

rulle-io requested a review from labianchin February 13, 2023 23:53

labianchin reviewed Apr 3, 2023

View reviewed changes

Ruslan Altynnikov added 3 commits May 16, 2023 23:23

Address some review comments.

ed93f13

Add more final parameters.

ef39bed

Add more final parameters.

a754434

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SQL column with NOT NULL constraint #581

Support SQL column with NOT NULL constraint #581

	i, sqlType, getSqlTypeName(sqlType), expectedAvroType);
	i, sqlType, JdbcAvroSchema.getSqlTypeName(sqlType), expectedAvroType);

Support SQL column with NOT NULL constraint #581

Are you sure you want to change the base?

Support SQL column with NOT NULL constraint #581

Conversation

Checklist for PR author(s)

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment