SQL Database DDL & Usability Improvements - #38952
Conversation
…SE and USE short syntax
|
/gemini review |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the SQL DDL capabilities within Beam SQL, particularly for database management. It enables users to create and delete databases, and introduces more robust behavior for database deletion by implementing Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request makes the "DATABASE" keyword optional in the "USE" statement, changes the visibility of schema helper methods in "SqlDdlNodes" to public, and updates "InMemoryCatalog" to prevent dropping non-empty databases unless "CASCADE" is specified. Feedback on these changes includes a recommendation to conditionally clean up state in "InMemoryCatalog.dropDatabase" only if the database was successfully removed, and to assert the more specific "IllegalStateException" instead of "RuntimeException" in the unit tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request makes the 'DATABASE' keyword optional in the 'USE' statement, increases the visibility of helper methods in 'SqlDdlNodes' to public, and updates 'InMemoryCatalog' to support dropping databases with 'CASCADE' or throwing an exception if the database is not empty when 'CASCADE' is not specified. Corresponding unit tests have been added. Feedback on the changes suggests checking if the database exists in 'InMemoryCatalog.dropDatabase' before performing any operations to prevent side effects and inconsistent states.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…se and add tests for DROP DATABASE IF EXISTS
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request makes the 'DATABASE' keyword optional in the 'USE DATABASE' SQL syntax, updates 'InMemoryCatalog' to properly handle 'DROP DATABASE' operations (including cascade behavior and empty checks), and exposes helper methods in 'SqlDdlNodes' as public. Feedback is provided to optimize the 'dropDatabase' method by removing a redundant collection lookup, and to add a defensive null check to 'childSchema' now that it has public visibility.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…check in SqlDdlNodes, and make tests more precise
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request makes the DATABASE keyword optional in the USE statement and improves the database dropping logic in InMemoryCatalog by checking if the database is empty when dropping without CASCADE. It also adds corresponding unit tests. The review feedback highlights a potential issue where setting currentDatabase to null when the active database is dropped could lead to NullPointerExceptions in subsequent operations, suggesting resetting it to a default database or throwing a clear exception instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
R: @Abacn |
There was a problem hiding this comment.
Code Review
This pull request makes the 'DATABASE' keyword optional in the 'USE' SQL syntax, exposes schema utility methods as public with added null safety, and refactors 'InMemoryCatalog.dropDatabase' to correctly handle restrict and cascade behaviors. It also adds comprehensive unit tests for these new features. The review feedback suggests adding a null check for the 'database' parameter in 'InMemoryCatalog.dropDatabase' to prevent potential NullPointerExceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| public boolean dropDatabase(String database, boolean cascade) { | ||
| checkState(!cascade, "%s does not support CASCADE.", getClass().getSimpleName()); | ||
| MetaStore metaStore = metaStores.get(database); | ||
| if (!cascade && metaStore != null && !metaStore.getTables().isEmpty()) { | ||
| throw new IllegalStateException("Database '" + database + "' is not empty."); | ||
| } | ||
|
|
||
| boolean removed = databases.remove(database); | ||
| if (!removed) { | ||
| return false; | ||
| } | ||
| if (database.equals(currentDatabase)) { | ||
| currentDatabase = null; | ||
| } | ||
| return removed; | ||
| metaStores.remove(database); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming and prevent potential NullPointerExceptions, we should add a null check for the database parameter at the beginning of the dropDatabase method. If database is null, calling metaStores.get(database) or database.equals(currentDatabase) will throw an exception (especially if metaStores is backed by a ConcurrentHashMap which does not permit null keys).
public boolean dropDatabase(String database, boolean cascade) {
if (database == null) {
return false;
}
MetaStore metaStore = metaStores.get(database);
if (!cascade && metaStore != null && !metaStore.getTables().isEmpty()) {
throw new IllegalStateException("Database '" + database + "' is not empty.");
}
boolean removed = databases.remove(database);
if (!removed) {
return false;
}
if (database.equals(currentDatabase)) {
currentDatabase = null;
}
metaStores.remove(database);
return true;
}
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
* SQL Database DDL & Usability Improvements: Support CREATE/DROP DATABASE and USE short syntax * Address code review feedback: add guard in InMemoryCatalog.dropDatabase and add tests for DROP DATABASE IF EXISTS * Address new code review feedback: optimize InMemoryCatalog, add null check in SqlDdlNodes, and make tests more precise
2 participants
Description
This PR is split from #38866. It introduces comprehensive support for database DDL (Data Definition Language) and usability improvements in the Beam SQL CLI and
InMemoryCatalog.Key Changes
CREATE DATABASEandDROP DATABASEstatements.CASCADEandRESTRICToptions forDROP DATABASE:DROP DATABASE <db> RESTRICT(default) will fail if the database is not empty, preventing accidental data loss.DROP DATABASE <db> CASCADEwill automatically drop all tables within that database before dropping the database itself.USE <database_name>shortcut in the CLI, removing the mandatory requirement of theDATABASEkeyword (i.e. you no longer have to typeUSE DATABASE <db>).CREATE EXTERNAL TABLE db_1.my_table ...orINSERT INTO db_1.my_table ...), allowing you to manage and write to tables in non-active databases.BeamSqlCliDatabaseTest.javaverifying all DDL and database management features in the CLI.