Option to have null defaults for avro sink schema #21842

moulimukherjee · 2023-09-20T17:42:48Z

This adds a way to specify an option NULL DEFAULTS in the CREATE SINK sql. If this is set, then the generated avro schema will have nullable fields with default value of null.

Motivation

Fixes https://github.com/MaterializeInc/database-issues/issues/6549

Tips for reviewer

I added testdrive tests to check some nesting and various combination of how that flag could be specified. Please let me know what else will be valuable to test.

Checklist

This PR has adequate test coverage / QA involvement has been duly considered.
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
This PR includes the following user-facing behavior changes:

@guswynn

Fixes a bug introduced in MaterializeInc#21842 and detected by @guswynn here https://github.com/MaterializeInc/materialize/pull/21482/files#r1310695260 Fixes #21496 Signed-off-by: Petros Angelatos <[email protected]>

@guswynn

Fixes a bug introduced in MaterializeInc#21842 and detected by @guswynn here https://github.com/MaterializeInc/materialize/pull/21482/files#r1310695260 Fixes #21496 Signed-off-by: Petros Angelatos <[email protected]>

src/sql-lexer/src/keywords.txt

maddyblue

Parser/plan parts lgtm.

src/sql-parser/src/parser.rs

moulimukherjee · 2023-09-22T04:27:22Z

Moving to draft, looking into ci failures.

misc/python/materialize/checks/temporal_types.py

benesch · 2023-09-22T06:32:27Z

I like where we landed with NULL DEFAULTS. Feels right to me. Don't wait for my further review—out of bandwidth.

src/sql/src/plan/statement/ddl.rs

misc/python/materialize/checks/temporal_types.py

src/interchange/src/avro.rs

sploiselle · 2023-09-22T12:31:54Z

src/interchange/src/json.rs

@@ -77,7 +77,7 @@ impl fmt::Debug for JsonEncoder {
                "schema",
                &format!(
                    "{:?}",
-                    build_row_schema_json(&self.value_columns, "schema", &BTreeMap::new())
+                    build_row_schema_json(&self.value_columns, "schema", &BTreeMap::new(), false)


ditto here––using boolean params in the outermost API isn't ideal

I am leaving this one in. In the following avro updates, there will be more configs like user defined comments and eventually user defined default values which should live in a struct along with this flag.

src/interchange/src/json.rs

updated tests moar test

moar tests

Co-authored-by: Sean Loiselle <[email protected]>

refactored slightly update

moulimukherjee · 2023-09-22T22:40:38Z

Seeing a "the in-memory state of the catalog does not match its on-disk state" error. Not happening locally though. Checking what's causing it. https://buildkite.com/materialize/tests/builds/64317#018abe9b-c962-4aea-b8d2-b87209dfdee5/1438-1440

src/interchange/src/json.rs

umanwizard

Looks good to me!

Feel free to re-request review if you feel it's necessary after you mark this as non-draft.

src/interchange/src/avro/encode.rs

src/interchange/src/json.rs

Co-authored-by: umanwizard <[email protected]>

moulimukherjee · 2023-09-26T00:24:49Z

Addressed the feedback, set this to auto-merge.

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

@guswynn

Fixes a bug introduced in MaterializeInc#21842 and detected by @guswynn here https://github.com/MaterializeInc/materialize/pull/21482/files#r1310695260 Fixes #21496 Signed-off-by: Petros Angelatos <[email protected]>

moulimukherjee force-pushed the support-automatic-nullable-default branch 2 times, most recently from 42f7e04 to 86c615c Compare September 21, 2023 22:08

moulimukherjee changed the title ~~WIP: Default nulls for avro sink schema~~ Option to have null defaults for avro sink schema Sep 21, 2023

moulimukherjee marked this pull request as ready for review September 21, 2023 22:25

moulimukherjee requested a review from a team September 21, 2023 22:25

moulimukherjee requested a review from a team as a code owner September 21, 2023 22:25

moulimukherjee requested review from a team, maddyblue, umanwizard and benesch September 21, 2023 22:25

moulimukherjee commented Sep 21, 2023

View reviewed changes

src/sql-lexer/src/keywords.txt Show resolved Hide resolved

maddyblue reviewed Sep 21, 2023

View reviewed changes

src/sql-parser/src/parser.rs Show resolved Hide resolved

moulimukherjee changed the title ~~Option to have null defaults for avro sink schema~~ WIP: Option to have null defaults for avro sink schema Sep 22, 2023

moulimukherjee marked this pull request as draft September 22, 2023 04:27

moulimukherjee commented Sep 22, 2023

View reviewed changes

misc/python/materialize/checks/temporal_types.py Outdated Show resolved Hide resolved

benesch removed their request for review September 22, 2023 06:32

sploiselle reviewed Sep 22, 2023

View reviewed changes

umanwizard suggested changes Sep 22, 2023

View reviewed changes

src/interchange/src/json.rs Outdated Show resolved Hide resolved

moulimukherjee mentioned this pull request Sep 22, 2023

test: fixing nightlies for temporal precision tests #21895

Closed

5 tasks

moulimukherjee force-pushed the support-automatic-nullable-default branch from 32a5e40 to e347730 Compare September 22, 2023 19:53

Mouli Mukherjee and others added 5 commits September 22, 2023 12:54

Initial changes

31fdfe3

tests

381e4af

updated tests moar test

more tests

79f378e

moar tests

fixing tests

0697a4a

Update src/sql/src/plan/statement/ddl.rs

5f29a92

Co-authored-by: Sean Loiselle <[email protected]>

Mouli Mukherjee added 3 commits September 22, 2023 12:54

remove unnecessary code

890724e

added an array and map type

83aebe7

Adding assertions + moar tests

86fcbdc

refactored slightly update

moulimukherjee force-pushed the support-automatic-nullable-default branch from e347730 to 86fcbdc Compare September 22, 2023 19:54

refactored the AvroSchemaGenerator::new method

82ad657

umanwizard suggested changes Sep 25, 2023

View reviewed changes

src/interchange/src/json.rs Outdated Show resolved Hide resolved

moulimukherjee commented Sep 25, 2023

View reviewed changes

src/interchange/src/json.rs Show resolved Hide resolved

updating the logic to detect is_nullable_union

ec7a329

moulimukherjee force-pushed the support-automatic-nullable-default branch from 5e632a9 to ec7a329 Compare September 25, 2023 18:54

dropping the created objects to see if they help with the failing test

ef23a85

moulimukherjee mentioned this pull request Sep 25, 2023

WIP: limited test on main #21957

Closed

fixing tests

23d7057

umanwizard approved these changes Sep 25, 2023

View reviewed changes

src/interchange/src/avro/encode.rs Outdated Show resolved Hide resolved

src/interchange/src/json.rs Show resolved Hide resolved

moulimukherjee and others added 2 commits September 25, 2023 17:17

Update src/interchange/src/avro/encode.rs

d5d0f59

Co-authored-by: umanwizard <[email protected]>

addressing review comments

8ca3df7

moulimukherjee marked this pull request as ready for review September 26, 2023 00:20

moulimukherjee changed the title ~~WIP: Option to have null defaults for avro sink schema~~ Option to have null defaults for avro sink schema Sep 26, 2023

moulimukherjee enabled auto-merge (squash) September 26, 2023 00:24

moulimukherjee merged commit e20ae46 into MaterializeInc:main Sep 26, 2023

def- added a commit to def-/materialize that referenced this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks

214dd21

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

def- mentioned this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks #22010

Merged

5 tasks

def- added a commit to def-/materialize that referenced this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks

d1fe912

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

def- added a commit to def-/materialize that referenced this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks

9aeaf52

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

def- added a commit to def-/materialize that referenced this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks

0b4d18f

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

def- added a commit to def-/materialize that referenced this pull request Sep 27, 2023

tests: Add platform-check for NULL DEFAULTS sinks

9e52a75

Also added 2 small parsing checks Follow-up to MaterializeInc#21842 which I missed

morsapaes mentioned this pull request Nov 14, 2023

Kafka sink: support NULL DEFAULTS option MaterializeInc/terraform-provider-materialize#362

Open

petrosagg mentioned this pull request Jun 6, 2024

storage: only messages should drive generator capability #21499

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Option to have null defaults for avro sink schema #21842

Option to have null defaults for avro sink schema #21842

moulimukherjee commented Sep 20, 2023 •

edited

Loading

maddyblue left a comment

moulimukherjee commented Sep 22, 2023

benesch commented Sep 22, 2023

sploiselle Sep 22, 2023

moulimukherjee Sep 22, 2023

moulimukherjee commented Sep 22, 2023 •

edited

Loading

umanwizard left a comment

moulimukherjee commented Sep 26, 2023

Option to have null defaults for avro sink schema #21842

Option to have null defaults for avro sink schema #21842

Conversation

moulimukherjee commented Sep 20, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

maddyblue left a comment

Choose a reason for hiding this comment

moulimukherjee commented Sep 22, 2023

benesch commented Sep 22, 2023

sploiselle Sep 22, 2023

Choose a reason for hiding this comment

moulimukherjee Sep 22, 2023

Choose a reason for hiding this comment

moulimukherjee commented Sep 22, 2023 • edited Loading

umanwizard left a comment

Choose a reason for hiding this comment

moulimukherjee commented Sep 26, 2023

moulimukherjee commented Sep 20, 2023 •

edited

Loading

moulimukherjee commented Sep 22, 2023 •

edited

Loading