adapter,interchange,parser: Support avro sink comments #22019

moulimukherjee · 2023-09-27T18:29:47Z

Added implementation to

automatically add comments to generated avro schemas from materialized comments (via COMMENT ON)
allow user to specify documentation in the sql

Currently, COMMENT ON (in materialize) does not support adding comments for a column in a custom type and this PR also skips that. I will do a follow up to add support for that https://github.com/MaterializeInc/database-issues/issues/6696.

Motivation

Partially fixes https://github.com/MaterializeInc/database-issues/issues/6480, does not allow comments on columns in a custom type yet.

Tips for reviewer

Added inline comments.

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.
- doc/developer: add design doc for adding docs to Avro sink schemas #21564
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:
- I will add user facing documentation as a follow up

cc: @benesch @dseisun-materialize

moulimukherjee · 2023-10-05T18:51:35Z

doc/developer/design/20230903_avro_doc.md

-    column name is taken to be a name of a column in the top level of the
-    materialized view. Object names are looked up according to usual SQL name
-    resolution rules for the search path and active database.
+    format `[[db.]schema.]object.column`. Object names are looked up according to usual


Updated the implementatoin to keep it consistent with the naming resolution for the type below [[db.]schema.]object and with the COMMENT ON sql which expects the object to be specified. Also, sinks can't be queried and they don't have a relation desc.

moulimukherjee · 2023-10-05T18:54:15Z

test/testdrive/kafka-avro-sinks-doc-comments.td

+# Test Avro UPSERT sinks doc comments
+
+$ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr}
+ALTER SYSTEM SET enable_comment = true;


Note, the avro comments from materialize comments behaviour is also gated by this flag. If this flag is not set, users can't create comments in materialize and there will be no automatic documentation added in the generated avro schema.

This does not prevent the user from explicitly adding a DOC ON options in their create sink sql though.

This does not prevent the user from explicitly adding a DOC ON options in their create sink sql though.

We should add a separate feature flag for the use of DOC ON comments, so that we can properly follow the feature lifecycle for this feature. It should start out in private preview while we're validating its design and implementation with our preview customers. That way we can make breaking changes if necessary.

Sounds good. Let me add that.

Done! Put it behind enable_sink_doc_on_option

shepherdlybot · 2023-10-05T19:05:43Z

This PR has higher risk. Make sure to carefully review the file hotspots. In addition to having a knowledgeable reviewer, it may be useful to add observability and/or a feature flag. What's This?

Risk Score	Probability	Buggy File Hotspots
🔴 80 / 100	59%	3

Buggy File Hotspots:

File	Percentile
../session/vars.rs	98
../src/catalog.rs	100
../src/parser.rs	90

src/sql-parser/src/ast/defs/ddl.rs

umanwizard · 2023-10-05T20:01:42Z

Currently, COMMENT ON does not support adding comments for a column in a custom type and this PR also skips that

FWIW, this works fine in Postgres:

brennan=> create type t as (f1 text);
CREATE TYPE
brennan=> comment on column t.f1 is 'hello, world!';
COMMENT

moulimukherjee · 2023-10-05T20:03:54Z

@umanwizard Yeah (updated the text). It's just not implemented yet in materialize. I will do a follow up to add both comment on support and DOC ON COLUMN support for them. This PR was becoming a bit too big and they'll have overlapping logic.

(Tracking https://github.com/MaterializeInc/database-issues/issues/6696)

src/sql-parser/src/ast/defs/ddl.rs

maddyblue · 2023-10-05T21:22:27Z

src/sql-parser/src/parser.rs

+
+    fn parse_column_name(&mut self) -> Result<(RawItemName, Ident), ParserError> {
+        let start = self.peek_pos();
+        let mut item_name = self.parse_raw_name()?;


This differs from

materialize/src/sql-parser/src/parser.rs

Line 7764 in 8b47d4c

let mut identifiers = self.parse_identifiers()?;

which calls parse_identifiers. I thiiiink the one in this PR is correct because that's what AstInfo expects. @ParkMyCar fyi: comment on column should probably be taught to call this new function once it supports AstInfo.

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

maddyblue · 2023-10-05T21:27:35Z

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

-    (AvroValueFullname, String),
-    (NullDefaults, bool, Default(false))
-);
+/// Creating this by hand instead of using generate_extracted_config! macro


Can you open an issue about this so we don't forget? I think keeping everything in the macro is good, so we should eventually add enum support to it.

Sure 👍. I did not find any other similar use cases, so this could be an outlier.

filed https://github.com/MaterializeInc/materialize/issues/22213, will add it in the comment as well.

umanwizard

Changes to Avro schema generation look fine to me. Someone from Adapter should review the rest.

I think we could use an end-to-end test that verifies that other Avro-speaking tools (e.g. the official Java or Python Avro libraries) can properly interpret these schemas and see the docs on them. But I wouldn't think that should block this PR as we have nothing like that currently. @philip-stoev or @def- , what do you think?

src/interchange/src/json.rs

src/sql-parser/src/parser.rs

src/interchange/src/json.rs

def- · 2023-10-05T22:55:31Z

I think we could use an end-to-end test that verifies that other Avro-speaking tools (e.g. the official Java or Python Avro libraries) can properly interpret these schemas and see the docs on them. But I wouldn't think that should block this PR as we have nothing like that currently. @philip-stoev or @def- , what do you think?

Makes sense to have that. I'll try implementing that test on Friday, probably using Python Avro lib, since that's easiest from our side. I'd prefer if you can wait for the test.

maddyblue

Adapter parts lgtm.

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

benesch

I'm planning to take a look at this tomorrow as well! Might slip into the weekend but will get you a review by Monday morning at the latest.

philip-stoev · 2023-10-06T09:18:34Z

Makes sense to have that. I'll try implementing that test on Friday, probably using Python Avro lib, since that's easiest from our side. I'd prefer if you can wait for the test.

Instead of bringing another tool into the mix, can we simply issue a GET request against the schema service and fish for the required parts in the response?

moulimukherjee · 2023-10-06T22:20:29Z

Moving to draft to look into the tests

def- · 2023-10-06T22:38:40Z

The failure is:

Running validate() from <materialize.checks.sink.SinkComments object at 0x7f1d4a52bad0>
$ docker compose exec -T testdrive testdrive --kafka-addr=kafka:9092 --schema-registry-url=http://schema-registry:8081 --materialize-url=postgres://materialize@materialized:6875 --materialize-internal-url=postgres://materialize@materialized:6877 --variable-length-row-encoding --aws-endpoint=http://localstack:4566 --no-reset --default-timeout=300s --seed=1 --persist-blob-url=file:///mzdata/persist/blob --persist-consensus-url=postgres://root@materialized:26257?options=--search_path=consensus --var=replicas=1 --var=default-replica-size=4-4 --var=default-storage-size=4-1 --source=/home/deen/git/materialize/misc/python/materialize/checks/sink.py:691
Verifying contents of latest schema for subject "sink-sink-comments1-key" in the schema registry...
Verifying contents of latest schema for subject "sink-sink-comments2-key" in the schema registry...
Verifying contents of latest schema for subject "sink-sink-comments3-key" in the schema registry...
Verifying contents of latest schema for subject "sink-sink-comments1-value" in the schema registry...
^^^ +++
11:1: error: schema did not match
expected:
{"type":"record","name":"envelope","doc":"comment on view sink_source_comments_view","fields":[{"name":"before","type":["null",{"type":"record","name":"row","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null,"doc":"doc on l_v1"},{"name":"l_v2","type":["null","long"],"default":null,"doc":"value doc on l_v1"},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]}

actual:
{"type":"record","name":"envelope","doc":"comment on view sink_source_comments_view","fields":[{"name":"before","type":["null",{"type":"record","name":"row","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null},{"name":"l_v2","type":["null","long"],"default":null},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]}
     |
  10 |
  11 | $ schema-registry-verify schema-type=avro subject=sink-sink-comments1-value
     | ^
+++ !!! Error Report

Maybe I'm holding it wrong?

moulimukherjee · 2023-10-06T22:57:53Z

@def- I had run into a similar issue locally because for column/fields the "doc" field wasn't getting serialized properly, I fixed that in this PR. Running mzcompose to check this out (taking a while).

Btw, is this running for a previous version? What scenario should I run this with?

moulimukherjee · 2023-10-06T23:09:44Z

@def- Oh I think it's the envelope DEBEZIUM. I could reproduce it in testdrive. Thanks for catching this! Will put up a fix.

moulimukherjee · 2023-10-07T03:16:18Z

@def- It should be fixed now.

def-

Thanks for waiting for my tests. I postponed the Python-avro-based check until we reach a conclusion on how we want to test it.

def- · 2023-10-09T10:50:39Z

misc/python/materialize/checks/sink.py


                $ schema-registry-verify schema-type=avro subject=sink-sink-comments2-value
-                {"type":"record","name":"envelope","doc":"comment on view sink_source_comments_view","fields":[{"name":"before","type":["null",{"type":"record","name":"row","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null,"doc":"doc on l_v1"},{"name":"l_v2","type":["null","long"],"default":null,"doc":"value doc on l_v1"},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]}
+                {"type":"record","name":"envelope","fields":[{"name":"before","type":["null",{"type":"record","name":"row","doc":"comment on view sink_source_comments_view","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null,"doc":"doc on l_v1"},{"name":"l_v2","type":["null","long"],"default":null,"doc":"value doc on l_v2"},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]}


Is it expected to have the comment on the row instead of on the envelope?

Yes, see discussion here: https://materializeinc.slack.com/archives/CUFU852KT/p1696642943413989

umanwizard · 2023-10-09T15:34:17Z

Dennis Felsing ***@***.***> writes:

@def- commented on this pull request. > $ schema-registry-verify schema-type=avro subject=sink-sink-comments2-value - {"type":"record","name":"envelope","doc":"comment on view sink_source_comments_view","fields":[{"name":"before","type":["null",{"type":"record","name":"row","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null,"doc":"doc on l_v1"},{"name":"l_v2","type":["null","long"],"default":null,"doc":"value doc on l_v1"},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]} + {"type":"record","name":"envelope","fields":[{"name":"before","type":["null",{"type":"record","name":"row","doc":"comment on view sink_source_comments_view","fields":[{"name":"l_k","type":"string"},{"name":"l_v1","type":["null","string"],"default":null,"doc":"doc on l_v1"},{"name":"l_v2","type":["null","long"],"default":null,"doc":"value doc on l_v2"},{"name":"c","type":"long"}]}],"default":null},{"name":"after","type":["null","row"],"default":null}]} Is it expected to have the comment on the row instead of on the envelope?

Yes. Discussed in Slack: https://materializeinc.slack.com/archives/CUFU852KT/p1696642943413989

…

-- Reply to this email directly or view it on GitHub: #22019 (review) You are receiving this because you were mentioned. Message ID: ***@***.***>

Addressing review comments updated error message Moar test

test updating test

platform-checks: Explicit sink comment check (currently fails) platform-checks: Added comments to Identifiers check parallel-workload: Enabled COMMENT ON testdrive: Added failure case on NULL and escaping sqlsmith: will come separately in MaterializeInc/sqlsmith#3

benesch

I only had time to quickly skim, but looks really solid. Basically no substantive comments. 👌🏽

benesch · 2023-10-10T06:27:27Z

test/testdrive/kafka-avro-sinks-doc-comments.td

+# Test Avro UPSERT sinks doc comments
+
+$ postgres-execute connection=postgres://mz_system:materialize@${testdrive.materialize-internal-sql-addr}
+ALTER SYSTEM SET enable_comment = true;


This does not prevent the user from explicitly adding a DOC ON options in their create sink sql though.

We should add a separate feature flag for the use of DOC ON comments, so that we can properly follow the feature lifecycle for this feature. It should start out in private preview while we're validating its design and implementation with our preview customers. That way we can make breaking changes if necessary.

benesch · 2023-10-10T06:33:39Z

doc/developer/design/20230903_avro_doc.md

-    column name is taken to be a name of a column in the top level of the
-    materialized view. Object names are looked up according to usual SQL name
-    resolution rules for the search path and active database.
+    format `[[db.]schema.]object.column`. Object names are looked up according to usual


def- · 2023-10-10T16:46:56Z

misc/python/materialize/mzcompose/__init__.py

@@ -69,6 +69,7 @@
    "persist_streaming_snapshot_and_fetch_enabled": "true",
    "enable_unified_clusters": "true",
    "enable_jemalloc_profiling": "true",
+    "enable_comment": "true",


should also add "enable_sink_doc_on_option": "true" here so we can use it easily in testing.

moulimukherjee · 2023-10-10T17:12:58Z

All the comments are addressed, enabling auto-merge.

Enabling flag for tests

Addressed the feedback, enabled flag for tests

moulimukherjee force-pushed the support-avro-comments branch from 589aa1c to ecfbee8 Compare September 27, 2023 20:31

def- requested review from def- and removed request for def- September 27, 2023 22:14

moulimukherjee mentioned this pull request Sep 28, 2023

storage: Fix testdrive test to not overwrite schema #22054

Merged

5 tasks

moulimukherjee force-pushed the support-avro-comments branch 4 times, most recently from cd4e56f to a626ba7 Compare October 5, 2023 15:58

moulimukherjee commented Oct 5, 2023

View reviewed changes

moulimukherjee changed the title ~~WIP: Support avro sink comments~~ Support avro sink comments Oct 5, 2023

moulimukherjee requested a review from def- October 5, 2023 18:58

moulimukherjee marked this pull request as ready for review October 5, 2023 19:02

moulimukherjee requested a review from a team October 5, 2023 19:02

moulimukherjee requested a review from a team as a code owner October 5, 2023 19:02

moulimukherjee requested a review from a team October 5, 2023 19:02

moulimukherjee requested a review from umanwizard as a code owner October 5, 2023 19:02

moulimukherjee requested a review from ParkMyCar October 5, 2023 19:02

moulimukherjee commented Oct 5, 2023

View reviewed changes

src/sql-parser/src/ast/defs/ddl.rs Outdated Show resolved Hide resolved

moulimukherjee requested a review from maddyblue October 5, 2023 19:43

maddyblue reviewed Oct 5, 2023

View reviewed changes

moulimukherjee force-pushed the support-avro-comments branch from 08ba4bf to 33453a5 Compare October 5, 2023 22:34

umanwizard reviewed Oct 5, 2023

View reviewed changes

maddyblue approved these changes Oct 6, 2023

View reviewed changes

src/sql/src/plan/statement/ddl.rs Outdated Show resolved Hide resolved

benesch reviewed Oct 6, 2023

View reviewed changes

def- force-pushed the support-avro-comments branch from e761c56 to 2464117 Compare October 6, 2023 22:45

moulimukherjee requested a review from def- October 7, 2023 03:17

moulimukherjee force-pushed the support-avro-comments branch from 165e417 to 83f3437 Compare October 7, 2023 04:49

moulimukherjee changed the title ~~WIP: adapter,interchange,parser: Support avro sink comments~~ adapter,interchange,parser: Support avro sink comments Oct 7, 2023

def- approved these changes Oct 7, 2023

View reviewed changes

def- reviewed Oct 9, 2023

View reviewed changes

Mouli Mukherjee and others added 4 commits October 9, 2023 21:20

Implementing avro doc comments

87eb196

Addressing review comments updated error message Moar test

Rebased from main to pull in changes for new ColumnName type

71488b2

test updating test

Fixing debezium case

388e025

moulimukherjee force-pushed the support-avro-comments branch from 83f3437 to 388e025 Compare October 10, 2023 04:20

benesch reviewed Oct 10, 2023

View reviewed changes

ParkMyCar approved these changes Oct 10, 2023

View reviewed changes

def- previously requested changes Oct 10, 2023

View reviewed changes

moulimukherjee force-pushed the support-avro-comments branch from 23e61d8 to 1bdcd5c Compare October 10, 2023 17:05

moulimukherjee requested a review from def- October 10, 2023 17:06

moulimukherjee enabled auto-merge (squash) October 10, 2023 17:13

Putting the doc on option behind feature flag

e393529

Enabling flag for tests

moulimukherjee force-pushed the support-avro-comments branch from 1bdcd5c to e393529 Compare October 10, 2023 17:44

moulimukherjee merged commit b414021 into MaterializeInc:main Oct 10, 2023

moulimukherjee deleted the support-avro-comments branch October 10, 2023 18:14

morsapaes mentioned this pull request Nov 14, 2023

Kafka sink: support DOC ON options MaterializeInc/terraform-provider-materialize#359

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adapter,interchange,parser: Support avro sink comments #22019

adapter,interchange,parser: Support avro sink comments #22019

moulimukherjee commented Sep 27, 2023 •

edited

Loading

moulimukherjee Oct 5, 2023 •

edited

Loading

benesch Oct 10, 2023

moulimukherjee Oct 5, 2023 •

edited

Loading

benesch Oct 10, 2023 •

edited

Loading

moulimukherjee Oct 10, 2023

moulimukherjee Oct 10, 2023

shepherdlybot bot commented Oct 5, 2023 •

edited

Loading

umanwizard commented Oct 5, 2023

moulimukherjee commented Oct 5, 2023 •

edited

Loading

maddyblue Oct 5, 2023

maddyblue Oct 5, 2023

moulimukherjee Oct 5, 2023

moulimukherjee Oct 5, 2023

umanwizard left a comment

def- commented Oct 5, 2023

maddyblue left a comment

benesch left a comment

philip-stoev commented Oct 6, 2023

moulimukherjee commented Oct 6, 2023

def- commented Oct 6, 2023

moulimukherjee commented Oct 6, 2023 •

edited

Loading

moulimukherjee commented Oct 6, 2023

moulimukherjee commented Oct 7, 2023

def- left a comment

def- Oct 9, 2023

benesch Oct 9, 2023

umanwizard commented Oct 9, 2023 via email

benesch left a comment

benesch Oct 10, 2023 •

edited

Loading

benesch Oct 10, 2023

def- Oct 10, 2023

moulimukherjee Oct 10, 2023

moulimukherjee commented Oct 10, 2023

adapter,interchange,parser: Support avro sink comments #22019

adapter,interchange,parser: Support avro sink comments #22019

Conversation

moulimukherjee commented Sep 27, 2023 • edited Loading

Motivation

Tips for reviewer

Checklist

moulimukherjee Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moulimukherjee Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

benesch Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepherdlybot bot commented Oct 5, 2023 • edited Loading

umanwizard commented Oct 5, 2023

moulimukherjee commented Oct 5, 2023 • edited Loading

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

umanwizard left a comment

Choose a reason for hiding this comment

def- commented Oct 5, 2023

maddyblue left a comment

Choose a reason for hiding this comment

benesch left a comment

Choose a reason for hiding this comment

philip-stoev commented Oct 6, 2023

moulimukherjee commented Oct 6, 2023

def- commented Oct 6, 2023

moulimukherjee commented Oct 6, 2023 • edited Loading

moulimukherjee commented Oct 6, 2023

moulimukherjee commented Oct 7, 2023

def- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umanwizard commented Oct 9, 2023 via email

benesch left a comment

Choose a reason for hiding this comment

benesch Oct 10, 2023 • edited Loading

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

moulimukherjee commented Oct 10, 2023

moulimukherjee commented Sep 27, 2023 •

edited

Loading

moulimukherjee Oct 5, 2023 •

edited

Loading

moulimukherjee Oct 5, 2023 •

edited

Loading

benesch Oct 10, 2023 •

edited

Loading

shepherdlybot bot commented Oct 5, 2023 •

edited

Loading

moulimukherjee commented Oct 5, 2023 •

edited

Loading

moulimukherjee commented Oct 6, 2023 •

edited

Loading

benesch Oct 10, 2023 •

edited

Loading