API: Add view interfaces

[jzhuge]

Co-authored-by: anjalinorwood@gmail.com

[jzhuge]

Split from #4567

[nastra]

@jzhuge I think you wanted to link to #4657 here :)

[amogh-jahagirdar]

Looks good to me, thanks @jzhuge for the contribution! @jackye1995 @nastra let us know if you have any further comments.

cc: @rdblue @danielcweeks

[jzhuge]

Thanks @amogh-jahagirdar.

BTW, I am also ok if we decide to remove UpdateViewProperties and just use the existing UpdateProperties.

[jackye1995]

Looks good to me!

[jackye1995]

@rdblue do you have any additional comment for adding these view APIs?

[jzhuge]

If no more comment, is it possible to get it merged so that I can use it in the Core PR?

[jzhuge]

Hi @danielcweeks, could you please take a look and merge if it looks ok?

[jzhuge]

Hi @danielcweeks, have you got a chance to take a look?

[danielcweeks]

You might want to remove references to genie.

[danielcweeks]

@jzhuge or @anjalinorwood I feel like I'm missing something in terms of where we actually access the view content / text? I would think that would live in ViewRepresentation or ViewVersion?

[amogh-jahagirdar]

@danielcweeks I'll let @jzhuge @anjalinorwood confirm but the model at least in my mind is that there is an implementation of ViewRepresentation; for the SQL view representation, this would be SqlViewRepresentation which would expose a sql() method for surfacing the literal text. Down the line there would be other view representation types such as for substrait which would expose the view representation in their own way (serialized plan node for example).

[jzhuge]

Merged Amogh's PR, rebased, and applied spotless.

[stevenzwu]

this is a getter. wondering why default...? should it just be catalog()?

[jzhuge]

In a Spark/Presto SQL session, you can run USE to set default catalog and namespace, thus the "default".

[jzhuge]

Created #6134 to add the missing field query-column-names to SQL view representation in the view spec.

[wmoustafa]

• No need for something like View.updateRepresentions(), as buildView() + replace() should be enough

Can one replace the current version or an old version by adding a new dialect? Does it result in a new version?

[wmoustafa]

My original question was that this builder returns a View, and in this discussion, you mentioned that withQuery() is the method responsible for the fact that it should return SQLViewRepresentation. However, I think this method still does not enforce it in this iteration.

[wmoustafa]

Why would not this method be also common with other representations?

[rdblue]

This is the SQL text of the view. Other view representations don't necessarily have a SQL query. I think this is reasonable.

[wmoustafa]

I feel this needs to be an Enum and somehow be merged with Type. @rdblue, @danielcweeks, @amogh-jahagirdar, thoughts? Further, it is better for the spec to take a position on supported dialects, or not take a position at all, but in this case SQL should not be a special type. Right now the position sounds a bit fuzzy.

[rdblue]

I disagree that we want to define supported dialects. We don't want engine writers to need to come to the Iceberg community and get a new symbol in an enum before storing a view. I don't see much benefit to doing that.

[wmoustafa]

I think it is fine to do either: not take a position at all or take a well defined position. There could be a few scenarios where the intended meaning is not achieved. For example:

• One defines the view SQL using a non-SQL string (e.g., some other DSL), and still leverages SQLViewReperesentation for that. Even if this is permitted, it is not clear from the spec if it is okay.
• The dialect could be the same but not standardized, e.g., dialects such as spark, Spark, SparkSQL, Spark SQL.

[wmoustafa]

Maybe what we need is an API to add supported dialects to the metadata? It is up to each catalog implementation to add their dialects, and view definitions can be associated with a dialect ID.

[jzhuge]

Maybe what we need is an API to add supported dialects to the metadata? It is up to each catalog implementation to add their dialects, and view definitions can be associated with a dialect ID.

Already added

  /**
   * Add a SQL {@link View} for a different dialect.
   *
   * @param sqlViewRepresentation a SQL view representation
   * @return this for method chaining
   * @throws IllegalArgumentException if the dialect is the same as the SQL view being built
   */
  ViewBuilder withOtherSQLRepresentation(SQLViewRepresentation sqlViewRepresentation);

[wmoustafa]

I was referring to making dialects (not just view representations) a construct in the metadata where people can define and add them, e.g., some catalogs define SparkSQL, id = 1 and TrinoSQL, id = 2, and each view representation references a standardized dialect ID. They are not part of the Iceberg spec, but defining and adding them dynamically is.

[rdblue]

One defines the view SQL using a non-SQL string

I don't think this is allowed. If you create a SQL view then it should contain SQL. In any case, being strict about dialects doesn't help prevent this.

The dialect could be the same but not standardized

This is a risk, but I think it isn't very likely. We should document the known dialect strings, but we shouldn't switch it to be an enum.

[jzhuge]

• No need for something like View.updateRepresentions(), as buildView() + replace() should be enough

Can one replace the current version or an old version by adding a new dialect? Does it result in a new version?

Not supported.

To add a new dialect, buildView() + withOtherSQLRepresentation() + replace()

[wmoustafa]

buildView() + withOtherSQLRepresentation() + replace() automatically copies existing dialects from previous versions or if we want to preserve them, we need to add them again?

[jzhuge]

buildView() + withOtherSQLRepresentation() + replace() automatically copies existing dialects from previous versions or if we want to preserve them, we need to add them again?

buildView() (with no argument) will start from scratch, i.e., we need to add everything again.

[rdblue]

@jzhuge, this looks good to go. Can you rebase and fix tests?

[jzhuge]

@rdblue Please merge.

[rdblue]

Merge! Let me know where the implementation PR is and I'll start looking at that!