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

Views: Clean up and clarify the view spec #7416

Merged
merged 2 commits into from
Apr 24, 2023

Conversation

rdblue
Copy link
Contributor

@rdblue rdblue commented Apr 23, 2023

This cleans up and clarifies a few places in the view spec. It is formatted to be more like the table spec.

| _optional_ | `default-catalog` | `string` | Catalog name to use when a reference in the SELECT does not contain a catalog |
| _optional_ | `default-namespace` | `list<string>` | Namespace to use when a reference in the SELECT is a single identifier |
| _optional_ | `field-aliases` | `list<string>` | Column names optionally specified in the create statement |
| _optional_ | `field-docs` | `list<string>` | Column descriptions (COMMENT) optionally specified in the create statement |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amogh-jahagirdar, this is currently field-comments in the implementation. Which one do you want to change?

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar Apr 24, 2023

Choose a reason for hiding this comment

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

Good catch, I'd prefer to change the spec to be field-comments. My rationale is that most operations (at least via SQL) will actually explicitly refer to the word "comment" so it seems more natural.

for example CREATE VIEW v (alias_name COMMENT 'docs', alias_name2, ...) AS SELECT col1, col2, ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Please have another look and I'll commit when you +1.

Copy link
Contributor

@amogh-jahagirdar amogh-jahagirdar left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @rdblue !

@rdblue rdblue merged commit e2c7e77 into apache:master Apr 24, 2023
@rdblue
Copy link
Contributor Author

rdblue commented Apr 24, 2023

Thanks, @amogh-jahagirdar!

@wmoustafa
Copy link
Contributor

I think it would be great to clarify a few things:

1- If we should document that version summary, namely engine and engine version, should have a correlation with the view dialect. For example, views created from Trino will typically be in Trino SQL.

2- If we should document whether a view version is immutable in terms of adding representations. Can an old (or committed) view version be changed to receive a new representation?

3- If we should document whether a view version can contain two definitions under the same type/dialect.

4- We discussed documenting possible values in the view interfaces PR. For example, similar to what we say that type must be sql, how about saying that dialect could be one of some options? This is just to avoid situations where there are dialects like spark, Spark, SparkSQL, spark3.3.

Happy to raise an issue or a community discussion around some of the above.

@rdblue
Copy link
Contributor Author

rdblue commented May 2, 2023

Thanks for reviewing this, @wmoustafa.

  1. I'm okay with this, but I don't think it is necessarily the case. Engines could support multiple dialects. Plus, I think it's pretty obvious that it will generally be true.
  2. View versions are immutable once created.
  3. Right now, there should not be duplicates for any representation type.
  4. I'm okay doing this, but it doesn't seem worthwhile to me. There will typically be one implementation per engine and people aren't going to change this often.

manisin pushed a commit to Snowflake-Labs/iceberg that referenced this pull request May 9, 2023
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.

3 participants