korlov42 commented on code in PR #6200:
URL: https://github.com/apache/ignite-3/pull/6200#discussion_r2194220057
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -162,10 +165,13 @@ public class DdlSqlToCommandConverter {
/** Zone options set. */
private final Set<String> knownZoneOptionNames;
+ /** Logical topology service for cluster wide storage profiles resolution.
*/
+ private final LogicalTopologyService logicalTopologyService;
Review Comment:
I would prefer to avoid direct reference of `LogicalTopologyService` in this
component just for the sake of validation. Let's introduce simple interface
like this
```
@FunctionalInterface
interface StorageProfileValidator {
void validate(Collection<String> storageProfiles);
}
```
and make `DdlSqlToCommandConverter` to accept it instead. For unit tests
purposes it will be no-op, for e2e and production it will be properly crafted
validator based on profiles from logical topology snapshot.
##########
modules/sql-engine/build.gradle:
##########
@@ -123,7 +123,8 @@ dependencies {
integrationTestImplementation libs.awaitility
integrationTestImplementation libs.netty.handler
integrationTestImplementation project(':ignite-api')
- integrationTestImplementation project(':ignite-schema')
+ integrationTestImplementation project(':ignite-catalog')
Review Comment:
catalog is already included (two lines below)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DistributionZoneSqlToCommandConverterTest.java:
##########
@@ -83,6 +91,18 @@ public static void setUp() {
assertThat(ZoneOptionEnum.values().length, is(NUMERIC_OPTIONS.size() +
STRING_OPTIONS.size()));
}
+ @BeforeEach
+ void prepareLogicalTopologyServiceMock() {
Review Comment:
I would expect unit test for added functionality in the first place
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/ddl/DdlSqlToCommandConverter.java:
##########
@@ -723,11 +731,56 @@ private CatalogCommand
convertCreateZone(IgniteSqlCreateZone createZoneNode, Pla
List<StorageProfileParams> profiles =
extractProfiles(createZoneNode.storageProfiles());
+ checkStorageProfilesArePresentedAmongCluster(profiles);
+
builder.storageProfilesParams(profiles);
return builder.build();
}
+ private void
checkStorageProfilesArePresentedAmongCluster(List<StorageProfileParams>
storageProfiles) {
+ LogicalTopologySnapshot localLogicalTopologySnapshot =
logicalTopologyService.localLogicalTopology();
+
+ String notPresentedStorageProfileName =
findStorageProfileNotPresentedInLogicalTopologySnapshot(
+ storageProfiles,
+ localLogicalTopologySnapshot
+ );
+
+ if (notPresentedStorageProfileName != null) {
+ throw new SqlException(STMT_VALIDATION_ERR, "Storage profile [" +
notPresentedStorageProfileName + "] doesn't exist.");
+ }
+ }
+
+ private static @Nullable String
findStorageProfileNotPresentedInLogicalTopologySnapshot(
+ List<StorageProfileParams> storageProfiles,
+ LogicalTopologySnapshot snapshot
+ ) {
+ Set<String> topologyWideProfiles =
extractStorageProfileNamesFromLogicalTopologySnapshot(snapshot);
+
+ for (StorageProfileParams profile : storageProfiles) {
+ String storageProfileName = profile.storageProfile();
+
+ if (!topologyWideProfiles.contains(storageProfileName)) {
Review Comment:
it would be nice to report all missing profiles, not just the first one
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/PrepareServiceImpl.java:
##########
@@ -152,13 +153,14 @@ public static PrepareServiceImpl create(
MetricManager metricManager,
SqlDistributedConfiguration clusterCfg,
SqlLocalConfiguration nodeCfg,
- SqlSchemaManager schemaManager
+ SqlSchemaManager schemaManager,
+ LogicalTopologyService logicalTopologyService
Review Comment:
since we are going to introduce one more parameter here, let it be
`DdlSqlToCommandConverter`. Also, new param is missing in javadoc.
--
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: notifications-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
