Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,92 @@ View createView(
String[] columnComments,
Map<String, String> properties) throws ViewAlreadyExistsException, NoSuchNamespaceException;

/**
* Replace a view in the catalog.
* <p>
* The default implementation has a race condition.
* Catalogs are encouraged to implement this operation atomically.
*
* @param ident a view identifier
* @param sql the SQL text that defines the view
* @param currentCatalog the current catalog
* @param currentNamespace the current namespace
* @param schema the view query output schema
* @param queryColumnNames the query column names
* @param columnAliases the column aliases
* @param columnComments the column comments
* @param properties the view properties
* @throws NoSuchViewException If the view doesn't exist or is a table
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
default void replaceView(
Identifier ident,
String sql,
String currentCatalog,
String[] currentNamespace,
StructType schema,
String[] queryColumnNames,
String[] columnAliases,
String[] columnComments,
Map<String, String> properties) throws NoSuchViewException, NoSuchNamespaceException {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we have a class ViewInfo to wrap these fields? It looks verbose to make so many parameters in 3 methods.

Copy link
Member

Choose a reason for hiding this comment

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

Agree, it is verbose.

if (viewExists(ident)) {
dropView(ident);
try {
createView(ident, sql, currentCatalog, currentNamespace, schema,
queryColumnNames, columnAliases, columnComments, properties);
}
catch (ViewAlreadyExistsException e) {
throw new RuntimeException("Race condition when dropping and creating view", e);
}
} else {
throw new NoSuchViewException(ident);
}
}

/**
* Create or replace a view in the catalog.
* <p>
* The default implementation has race conditions.
* Catalogs are encouraged to implement this operation atomically.
*
* @param ident a view identifier
* @param sql the SQL text that defines the view
* @param currentCatalog the current catalog
* @param currentNamespace the current namespace
* @param schema the view query output schema
* @param queryColumnNames the query column names
* @param columnAliases the column aliases
* @param columnComments the column comments
* @param properties the view properties
* @throws NoSuchNamespaceException If the identifier namespace does not exist (optional)
*/
default void createOrReplaceView(
Copy link
Contributor

Choose a reason for hiding this comment

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

3 APIs might be too much. How about we follow the create/replace table logical plan? We have a CreateTable with a bool field ignoreIfExists, and a ReplaceTable with a bool field orCreate.

So we can have just two methods here

def createView(... , ignoreIfExists: Boolean)

def replaceView(..., orCreate: Boolean)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the feedback @cloud-fan. I'll follow up with your suggestions and will ping you for a review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cloud-fan I have made the suggested improvements in #44330 but I'm not sure about having ignoreIfExists on the API when creating a view.
I checked and the TableCatalog also doesn't have such functionality and I wonder if it wouldn't be better to have ignoreIfExists be part of the physical plan that creates the view (similar to how it's done in https://github.com/apache/spark/pull/44197/files#diff-d491b3b4f1ac890067f57f4c30cbdecf3585ea91dabf0dced59b31a4e1643462R83)

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense. A simpler API is better. The caller side (Spark) can just ignore the error if ignoreIfExists is true.

Identifier ident,
String sql,
String currentCatalog,
String[] currentNamespace,
StructType schema,
String[] queryColumnNames,
String[] columnAliases,
String[] columnComments,
Map<String, String> properties) throws NoSuchNamespaceException {
if (viewExists(ident)) {
try {
replaceView(ident, sql, currentCatalog, currentNamespace, schema,
queryColumnNames, columnAliases, columnComments, properties);
} catch (NoSuchViewException e) {
throw new RuntimeException("Race condition when checking and replacing view", e);
}
} else {
try {
createView(ident, sql, currentCatalog, currentNamespace, schema,
queryColumnNames, columnAliases, columnComments, properties);
} catch (ViewAlreadyExistsException e) {
throw new RuntimeException("Race condition when checking and creating view", e);
}
}
}

/**
* Apply {@link ViewChange changes} to a view in the catalog.
* <p>
Expand Down