Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support SQL column with NOT NULL constraint #581

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

rulle-io
Copy link
Contributor

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.

Ruslan Altynnikov added 3 commits January 22, 2023 17:29
Turned out that classes BaseFieldTypeBuilder & BaseTypeBuilder are unrelated.
```
public static final class FieldTypeBuilder<R> extends BaseFieldTypeBuilder<R> {
public static class BaseTypeBuilder<R> {
```
@codecov
Copy link

codecov bot commented Feb 11, 2023

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     

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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Avoiding static import.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a breaking change?

No, by default it has the same behaviour (backwards compatible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants