ptupitsyn commented on code in PR #4452:
URL: https://github.com/apache/ignite-3/pull/4452#discussion_r1776707848
##########
modules/api/src/main/java/org/apache/ignite/catalog/ColumnType.java:
##########
@@ -352,4 +352,6 @@ private ColumnType<T> length_(Integer length) {
this.length = length;
return this;
}
+
Review Comment:
Please revert.
##########
modules/api/src/main/java/org/apache/ignite/catalog/IgniteCatalog.java:
##########
@@ -191,6 +191,22 @@ public interface IgniteCatalog {
*/
Table createTable(TableDefinition definition);
+ /**
+ * Returns definition of the table with provided name.
Review Comment:
What if the table does not exist - does it return null or throw an exception?
##########
modules/catalog-dsl/src/integrationTest/java/org/apache/ignite/internal/matcher/ColumnDefinitionsMatcher.java:
##########
@@ -0,0 +1,43 @@
+package org.apache.ignite.internal.matcher;
Review Comment:
Missing license header - here and below.
##########
modules/api/src/main/java/org/apache/ignite/catalog/IgniteCatalog.java:
##########
@@ -205,6 +221,10 @@ public interface IgniteCatalog {
*/
void createZone(ZoneDefinition definition);
+ CompletableFuture<ZoneDefinition> zoneDefinitionAsync(String zoneName);
+
+ ZoneDefinition zoneDefinition(String zoneName);
Review Comment:
Missing javadoc.
##########
modules/api/src/main/java/org/apache/ignite/catalog/definitions/ZoneDefinition.java:
##########
@@ -176,6 +176,22 @@ public Builder toBuilder() {
return new Builder(this);
}
+ @Override
+ public String toString() {
+ return "ZoneDefinition{"
+ + "zoneName='" + zoneName + '\''
+ + ", ifNotExists=" + ifNotExists
+ + ", partitions=" + partitions
+ + ", replicas=" + replicas
+ + ", distributionAlgorithm='" + distributionAlgorithm + '\''
+ + ", dataNodesAutoAdjust=" + dataNodesAutoAdjust
+ + ", dataNodesAutoAdjustScaleUp=" + dataNodesAutoAdjustScaleUp
+ + ", dataNodesAutoAdjustScaleDown=" +
dataNodesAutoAdjustScaleDown
+ + ", filter='" + filter + '\''
+ + ", storageProfiles='" + storageProfiles + '\''
+ + '}';
Review Comment:
```suggestion
return IgniteToStringBuilder.toString(ZoneDefinition.class, this);
```
##########
modules/api/src/main/java/org/apache/ignite/lang/DistributionZoneNotFoundException.java:
##########
@@ -15,18 +15,17 @@
* limitations under the License.
*/
-package org.apache.ignite.internal.distributionzones.exception;
+package org.apache.ignite.lang;
import static
org.apache.ignite.lang.ErrorGroups.DistributionZones.ZONE_NOT_FOUND_ERR;
import java.util.UUID;
-import org.apache.ignite.internal.lang.IgniteInternalException;
import org.jetbrains.annotations.Nullable;
/**
* Exception is thrown when appropriate distribution zone can`t be found.
*/
-public class DistributionZoneNotFoundException extends IgniteInternalException
{
+public class DistributionZoneNotFoundException extends IgniteException {
Review Comment:
I don't think we should make this public. The user should get `null` result
when a zone does not exist.
##########
modules/catalog-dsl/src/main/java/org/apache/ignite/internal/catalog/sql/SelectFromView.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.catalog.sql;
+
+import static
org.apache.ignite.internal.util.CompletableFutures.nullCompletedFuture;
+
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
+import java.util.concurrent.CompletableFuture;
+import java.util.function.Function;
+import org.apache.ignite.sql.IgniteSql;
+import org.apache.ignite.sql.SqlRow;
+import org.apache.ignite.sql.async.AsyncResultSet;
+
+class SelectFromView<T> extends AbstractCatalogQuery<List<T>> {
+ private final String viewName;
+
+ private final List<Option> whereOptions = new ArrayList<>();
+
+ private final Function<SqlRow, T> mapper;
+
+ SelectFromView(IgniteSql sql, String viewName, Option whereOption,
Function<SqlRow, T> mapper) {
+ this(sql, viewName, List.of(whereOption), mapper);
+ }
+
+ SelectFromView(IgniteSql sql, String viewName, List<Option> whereOptions,
Function<SqlRow, T> mapper) {
+ super(sql);
+ this.viewName = viewName;
+ this.whereOptions.addAll(whereOptions);
+ this.mapper = mapper;
+ }
+
+ @Override
+ public CompletableFuture<List<T>> executeAsync() {
+ return sql.executeAsync(null, toString()).thenCompose(resultSet -> {
+ List<T> result = new ArrayList<>();
+ return iterate(resultSet, result).thenApply(unused -> result);
+ });
+ }
+
+ private CompletableFuture<Void> iterate(AsyncResultSet<SqlRow> resultSet,
List<T> result) {
+ for (SqlRow row : resultSet.currentPage()) {
+ result.add(mapper.apply(row));
+ }
+ if (resultSet.hasMorePages()) {
+ return resultSet.fetchNextPage().thenCompose(nextPage ->
iterate(nextPage, result));
+ } else {
+ return nullCompletedFuture();
+ }
+ }
+
+ @Override
+ // Noop
+ protected List<T> result() {
+ return Collections.emptyList();
+ }
+
+ @Override
+ protected void accept(QueryContext ctx) {
+ ctx.sql("SELECT * FROM SYSTEM." + viewName + " ");
Review Comment:
Let's select only the columns we actually need. We should never use `SELECT
*` in production code.
##########
modules/catalog-dsl/build.gradle:
##########
@@ -26,6 +26,7 @@ dependencies {
implementation libs.jetbrains.annotations
implementation project(':ignite-api')
implementation project(':ignite-core')
+ implementation project(':ignite-catalog')
Review Comment:
Please remove, this brings a lot of extra dependencies to `ignite-client`,
such as `ignite-configuration` and `ignite-vault` - not acceptable.
--
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]
