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

docs: Fix grammar of mat view WITH options #27325

Merged
merged 2 commits into from
Jun 18, 2024

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented May 28, 2024

Fixes two issues with the grammar diagram of CREATE MATERIALIZED VIEW:

  • It looked as if WITH () (i.e., empty parens after WITH) would be possible.
  • It looked as if ASSERT NOT NULL and RETAIN HISTORY can come only in this order, but actually ASSERT NOT NULL can be specified also after RETAIN HISTORY (even both before and after).

Note that the new diagram is also not perfect, because now it looks as if RETAIN HISTORY could be specified more than once. I think we have to accept this, because it seems impossible to exactly specify using BNF that these options can be specified in any order, and ASSERT NOT NULL can be specified any number of times, but RETAIN HISTORY can only be specified once. I think it's ok that we are not capturing certain restrictions in the grammar diagram, especially if they are handled by planning and not by parsing. (For example, it's also impossible to capture that ASSERT NOT NULL can't be specified for the same column twice.)

Also, the new layout seems cleaner overall, especially when I add REFRESH soon.

Note that I did not add the optional = for ASSERT NOT NULL. Started a discussion on this here:
https://materializeinc.slack.com/archives/C063H5S7NKE/p1716888019218749

Plus the commit removes a stale PrPr notice fragment for ASSERT NOT NULL.

Next I'm going to work on adding REFRESH, but I'd like to get an opinion on the above before I get too deep into the REFRESH grammar.

Motivation

Fixes minor docs issues.

Tips for reviewer

Checklist

@ggevay ggevay added the A-docs Area: documentation label May 28, 2024
@ggevay ggevay requested a review from morsapaes as a code owner May 28, 2024 12:26
@ggevay ggevay requested a review from a team as a code owner June 2, 2024 17:12
@ggevay
Copy link
Contributor Author

ggevay commented Jun 2, 2024

Cherry-picked @arusahni's commit from here to make the hugo build work on newer hugo. It works for me now on hugo v0.126.3.

Copy link
Contributor

@def- def- left a comment

Choose a reason for hiding this comment

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

No complaints from QA side

Fixes two issues:
- It looked as if `WITH ()` would be possible.
- It looked as if `ASSERT NOT NULL` and `RETAIN HISTORY` can come
  only in this order, but actually `ASSERT NOT NULL` can be specified
  also after `RETAIN HISTORY` (even both before and after).

Note that the new diagram is also not perfect, because now it looks as
if `RETAIN HISTORY` could be specified more than once. I think we have
to accept this, because it seems impossible to exactly specify using
BNF that these options can be specified in any order, and `ASSERT NOT
NULL` can be specified any number of times, but `RETAIN HISTORY` can
only be specified once.

Also, the new layout seems cleaner overall, especially when I add
REFRESH soon.

Note that I did not add the optional `=` for `ASSERT NOT NULL`. Started
a discussion on this here:
https://materializeinc.slack.com/archives/C063H5S7NKE/p1716888019218749

Plus the commit removes a stale PrPr notice fragment for
ASSERT NOT NULL.
@ggevay ggevay force-pushed the mat-view-grammar branch 2 times, most recently from 586fe17 to 35041de Compare June 7, 2024 14:00
@ggevay
Copy link
Contributor Author

ggevay commented Jun 7, 2024

CI is failing in "Lint docs" for some reason:
https://buildkite.com/materialize/test/builds/83401#018ff2fc-8d73-4f4d-8137-d6aa7a9e4a65

I've now tried to temporarily remove Aru's commit that upgrades Hugo to see if that is the reason or my commit.

@ggevay
Copy link
Contributor Author

ggevay commented Jun 7, 2024

"Lint docs" has now passed: https://buildkite.com/materialize/test/builds/83403#018ff300-cc6b-47b9-91bf-af9b01e0c43b
So it seems to be @arusahni's commit from that I cherry-picked earlier which failed the linting. I guess that could be worked on in a separate PR, independently from this one.

@arusahni
Copy link
Contributor

arusahni commented Jun 7, 2024

My recollection of that PR is coming back. The Hugo upgrade brings about a change in how it computes URLs. Marta and I chatted about this and decided we probably should go for the workaround described here as the cross reference syntax isn't desirable for our docs.

@ggevay ggevay mentioned this pull request Jun 7, 2024
5 tasks
@morsapaes
Copy link
Contributor

I think there was a mistake when merging the RETAIN HISTORY documentation, because this should use a generic diagram for WITH options which doesn't spell out the options you can use in the diagram itself, but just points out the generic syntax:

Screenshot 2024-06-18 at 03 18 29

The options are then laid out in the respective configuration option table:

Screenshot 2024-06-18 at 03 21 11

We should also include examples for each configuration option since these diagrams are impossible to read, anyway.


I'll push up a fix. Pinged @chaas because this is an issue in a lot of reference pages, and we should patch it more generally.

@ggevay
Copy link
Contributor Author

ggevay commented Jun 18, 2024

Ok, this makes a lot of sense! Thank you!

@ggevay ggevay merged commit d69e9f5 into MaterializeInc:main Jun 18, 2024
11 checks passed
@ggevay
Copy link
Contributor Author

ggevay commented Jun 18, 2024

Btw. are the simplifications compared to the generic WITH option diagram from your above comment intentional? There is only WITH with_options on the diagram now, which doesn't show the parenthesis from the WITH (...) and doesn't show that there should be a comma between the options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants