Copilot commented on code in PR #11188:
URL: https://github.com/apache/gravitino/pull/11188#discussion_r3295984858
##########
core/src/main/java/org/apache/gravitino/catalog/TableOperationDispatcher.java:
##########
@@ -81,7 +84,24 @@ public class TableOperationDispatcher extends
OperationDispatcher implements Tab
*/
public TableOperationDispatcher(
CatalogManager catalogManager, EntityStore store, IdGenerator
idGenerator) {
+ this(catalogManager, store, idGenerator, () ->
GravitinoEnv.getInstance().schemaDispatcher());
+ }
+
+ /**
+ * Creates a new TableOperationDispatcher instance.
+ *
+ * @param catalogManager The CatalogManager instance to be used for table
operations.
+ * @param store The EntityStore instance to be used for table operations.
+ * @param idGenerator The IdGenerator instance to be used for table
operations.
+ * @param schemaDispatcherSupplier The SchemaDispatcher supplier to ensure
schemas are imported.
+ */
+ public TableOperationDispatcher(
+ CatalogManager catalogManager,
+ EntityStore store,
+ IdGenerator idGenerator,
+ Supplier<SchemaDispatcher> schemaDispatcherSupplier) {
super(catalogManager, store, idGenerator);
+ this.schemaDispatcherSupplier = schemaDispatcherSupplier;
Review Comment:
The new constructor accepts `schemaDispatcherSupplier` but does not validate
it (or its returned value). If a caller passes `null` (or a supplier that
returns `null`), later calls to `loadTable`/`createTable` will fail with a bare
NPE. Consider adding a `Preconditions.checkNotNull(schemaDispatcherSupplier,
...)` in the constructor and/or a `checkState`/`checkNotNull` around
`schemaDispatcherSupplier.get()` where it is used to provide a clearer failure
mode.
##########
core/src/main/java/org/apache/gravitino/catalog/ViewOperationDispatcher.java:
##########
@@ -69,7 +72,24 @@ public class ViewOperationDispatcher extends
OperationDispatcher implements View
*/
public ViewOperationDispatcher(
CatalogManager catalogManager, EntityStore store, IdGenerator
idGenerator) {
+ this(catalogManager, store, idGenerator, () ->
GravitinoEnv.getInstance().schemaDispatcher());
+ }
+
+ /**
+ * Creates a new ViewOperationDispatcher instance.
+ *
+ * @param catalogManager The CatalogManager instance to be used for view
operations.
+ * @param store The EntityStore instance to be used for view operations.
+ * @param idGenerator The IdGenerator instance to be used for view
operations.
+ * @param schemaDispatcherSupplier The SchemaDispatcher supplier to ensure
schemas are imported.
+ */
+ public ViewOperationDispatcher(
+ CatalogManager catalogManager,
+ EntityStore store,
+ IdGenerator idGenerator,
+ Supplier<SchemaDispatcher> schemaDispatcherSupplier) {
super(catalogManager, store, idGenerator);
+ this.schemaDispatcherSupplier = schemaDispatcherSupplier;
Review Comment:
The new constructor accepts `schemaDispatcherSupplier` but does not validate
it (or its returned value). If a caller passes `null` (or a supplier that
returns `null`), `loadView`/`createView` will later fail with a NPE. Consider
adding a `Preconditions.checkNotNull(schemaDispatcherSupplier, ...)` in the
constructor and/or validating `schemaDispatcherSupplier.get()` at use sites so
failures are explicit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
