Skip to content

Conversation

@jzhuge
Copy link
Member

@jzhuge jzhuge commented Nov 7, 2022

Current view spec misses the field query-column-names in SQL view representation.

For SELECT star view queries, the schema for the underlying table or view may change after the view has been created.
Thus, we need to store the column names of the view query, because when using the view, it is better to pick the columns
according to the name and order when the view was created and omit the extra columns we don't require.

@jzhuge jzhuge mentioned this pull request Nov 7, 2022
Comment on lines +130 to +131
according to the name and order when the view was created and omit the extra columns we don't require.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is not there a chance for them to become ambiguous after evolution? Most query engine just fail the query if the underlying tables evolve in a way that changes the number of columns a * expands to.

Copy link
Contributor

Choose a reason for hiding this comment

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

So if you create a SELECT * view on tables t1 with columns (a, b) joined with table t2with columns(x), then add column atot2, the query text will be ambiguous, and in fact I tried it on Spark and it threw an exception when querying the view: ```The SQL query of view db1.v` has an incompatible schema change and column a cannot be resolved. Expected 1 columns named a but got [a,a]```

The above sounds like an implementation detail of Spark, and probably should not be exposed by Iceberg. One solution to work around it is to record table names as well, not just view names. Another is to expect to expand the query text (i.e., expand the *) at view creation time, like what Hive does.

| Required | dialect | A string specifying the dialect of the ‘sql’ field. It can be used by the engines to detect the SQL dialect. |
| Optional | schema-id | ID of the view's schema when the version was created |
| Optional | default-catalog | A string specifying the catalog to use when the table or view references in the view definition do not contain an explicit catalog. |
| Optional | default-namespace | The namespace to use when the table or view references in the view definition do not contain an explicit namespace. Since the namespace may contain multiple parts, it is serialized as a list of strings. |
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: references -> referenced (d at the end) ?

@rdblue
Copy link
Contributor

rdblue commented Nov 27, 2022

I don't think that query column names are the right way to go. I think that the schema referenced by ID should be the pre-alias schema. That supports getting the pre-alias column names as well as more validation on data types and nested fields.

@rdblue rdblue closed this Nov 27, 2022
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.

4 participants