korlov42 commented on a change in pull request #309:
URL: https://github.com/apache/ignite-3/pull/309#discussion_r703538442
##########
File path:
modules/core/src/main/java/org/apache/ignite/internal/util/CollectionUtils.java
##########
@@ -45,6 +45,16 @@ public static boolean nullOrEmpty(@Nullable Collection<?>
col) {
return col == null || col.isEmpty();
}
+ /**
+ * Tests if the given array is either {@code null} or empty.
+ *
+ * @param col Array to check.
+ * @return Whether or not the given array is {@code null} or empty.
+ */
+ public static <T> boolean nullOrEmpty(@Nullable T[] col) {
Review comment:
Actually such method already exists. Please see
`org.apache.ignite.internal.util.ArrayUtils#nullOrEmpty(T[])`
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DropTableCommand.java
##########
@@ -20,13 +20,10 @@
/**
* DROP TABLE statement.
*/
-public class DropTableCommand implements DdlCommand {
+public class DropTableCommand extends DdlCommand {
/** Schema name. */
private String schemaName;
Review comment:
Why don't you move the schemaName to the DdlCommand as well?
##########
File path: modules/calcite/src/main/codegen/config.fmpp
##########
@@ -46,6 +50,7 @@ data: {
"BACKUPS"
Review comment:
key words TEMPLATE, AFFINITY_KEY, ATOMICITY, WRITE_SYNCHRONIZATION_MODE,
CACHE_GROUP, CACHE_NAME, DATA_REGION, VALUE_TYPE, ENCRYPTED are not a part of
grammar anymore. Please remove them from both `keywords` and
`nonReservedKeywords` groups
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DdlSqlToCommandConverter.java
##########
@@ -205,7 +198,7 @@ private CreateTableCommand
convertCreateTable(IgniteSqlCreateTable createTblNode
* Converts a given DropTable AST to a DropTable command.
*
* @param dropTblNode Root node of the given AST.
- * @param ctx Planning context.
+ * @param ctx Planning context.
Review comment:
space
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/IgniteTypeFactory.java
##########
@@ -126,6 +131,33 @@ else if (type instanceof BasicSqlType || type instanceof
IntervalSqlType) {
}
}
+ /**
+ * Gets ColumnType type for given class.
+ *
+ * @param relType Rel type.
+ * @return ColumnType type or null.
+ */
+ public ColumnType columnType(RelDataType relType) {
+ assert relType != null;
+
+ Type javaType = getResultClass(relType);
+
+ if (javaType == BigInteger.class) {
+ return relType.getPrecision() == PRECISION_NOT_SPECIFIED ?
ColumnType.numberOf() :
+ ColumnType.numberOf(relType.getPrecision());
+ }
+ else if (javaType == BigDecimal.class) {
+ if ((relType.getPrecision() == PRECISION_NOT_SPECIFIED &&
relType.getScale() != SCALE_NOT_SPECIFIED) ||
Review comment:
1) it's OK to specify precision only for decimal
2) I don't sure it's a proper place for such validation
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DdlSqlToCommandConverter.java
##########
@@ -178,11 +176,6 @@ private CreateTableCommand
convertCreateTable(IgniteSqlCreateTable createTblNode
.map(SqlKeyConstraint.class::cast)
.collect(Collectors.toList());
- if (pkConstraints.size() > 1)
Review comment:
why did you decide to delete this validation?
##########
File path:
modules/schema/src/main/java/org/apache/ignite/internal/schema/SchemaTableImpl.java
##########
@@ -72,6 +73,8 @@ public SchemaTableImpl(
) {
super(tableName);
+ Objects.requireNonNull(cols);
+
this.schemaName = schemaName;
this.cols = cols;
Review comment:
```suggestion
this.schemaName = schemaName;
this.cols = Objects.requireNonNull(cols);
```
##########
File path: modules/api/src/main/java/org/apache/ignite/schema/SchemaTable.java
##########
@@ -75,4 +76,9 @@
* @return Table modification builder.
*/
TableModificationBuilder toBuilder();
+
+ /** @return Table with schema canonical name. */
+ static String canonicalName(@NotNull String schema, @NotNull String name) {
Review comment:
I would prefer to not pollute an interface of Public API with an utility
methods
##########
File path: modules/api/src/main/java/org/apache/ignite/sql/IgniteSql.java
##########
@@ -0,0 +1,29 @@
+/*
+ * 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.sql;
+
+import java.util.List;
+import org.apache.ignite.internal.util.Cursor;
+
+/**
+ * Interface that provides methods for sql queries.
+ */
+public interface IgniteSql {
+ /** Sql query processor. */
Review comment:
please fix this javadoc
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DdlCommand.java
##########
@@ -17,5 +17,21 @@
package org.apache.ignite.internal.processors.query.calcite.prepare.ddl;
/** Common interface to group all DDL operations. */
-public interface DdlCommand {
+public abstract class DdlCommand {
Review comment:
not any DDL command is about table. So return the interface back please
##########
File path: modules/calcite/src/main/codegen/includes/parserImpls.ftl
##########
@@ -164,6 +164,62 @@ SqlCreate SqlCreateTable(Span s, boolean replace) :
}
}
Review comment:
please clean up `CreateTableOptionKey`
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementor.java
##########
@@ -463,7 +463,24 @@ else if (rel instanceof Intersect)
/** {@inheritDoc} */
@Override public Node<Row> visit(IgniteTableModify rel) {
- throw new UnsupportedOperationException();
Review comment:
better to revert this change. ModifyNode is already implemented
[here](https://github.com/apache/ignite-3/pull/295) (still open since it has
blocking issue)
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/ddl/DdlSqlToCommandConverter.java
##########
@@ -153,22 +140,33 @@ private CreateTableCommand
convertCreateTable(IgniteSqlCreateTable createTblNode
IgnitePlanner planner = ctx.planner();
- List<ColumnDefinition> cols = new ArrayList<>();
+ Column[] cols = new Column[colDeclarations.size()];
+
+ int colPos = 0;
+
+ IgniteTypeFactory typeFactory = ctx.typeFactory();
for (SqlColumnDeclaration col : colDeclarations) {
if (!col.name.isSimple())
throw new IgniteException("Unexpected value of columnName [" +
"expected a simple identifier, but was " + col.name + "; "
+
- "querySql=\"" + ctx.query() + "\"]"/*,
IgniteQueryErrorCode.PARSING*/);
+ "querySql=\"" + ctx.query() + "\"]");
String name = col.name.getSimple();
- RelDataType type = planner.convert(col.dataType);
+
+ RelDataType relType = planner.convert(col.dataType);
Object dflt = null;
if (col.expression != null)
dflt = ((SqlLiteral)col.expression).getValue();
- cols.add(new ColumnDefinition(name, type, dflt));
Review comment:
ColumnDefinition serves one small yet crucial purpose -- it narrows an
integration point to DdlCommandHandler only
##########
File path:
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -503,16 +503,16 @@ private void dropTableLocally(String name, UUID tblId,
List<List<ClusterNode>> a
final CompletableFuture<SchemaEventParameters> schemaReadyFut =
new CompletableFuture<>();
- CompletableFuture.allOf(schemaReadyFut)
+ schemaReadyFut
.exceptionally(e -> {
LOG.error("Failed to upgrade schema for a table [name=" +
tblName + ", id=" + tblId + ']', e);
- onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName), e);
+ onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName, tbl), e);
return null;
})
.thenRun(() ->
- onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName), null)
+ onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName, tbl), null)
Review comment:
```suggestion
onEvent(TableEvent.ALTER, new TableEventParameters(tbl),
null)
```
##########
File path:
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/type/IgniteTypeFactory.java
##########
@@ -126,6 +131,33 @@ else if (type instanceof BasicSqlType || type instanceof
IntervalSqlType) {
}
}
+ /**
+ * Gets ColumnType type for given class.
+ *
+ * @param relType Rel type.
+ * @return ColumnType type or null.
+ */
+ public ColumnType columnType(RelDataType relType) {
+ assert relType != null;
+
+ Type javaType = getResultClass(relType);
+
+ if (javaType == BigInteger.class) {
Review comment:
what about String and Bytes types?
##########
File path:
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/TableManager.java
##########
@@ -503,16 +503,16 @@ private void dropTableLocally(String name, UUID tblId,
List<List<ClusterNode>> a
final CompletableFuture<SchemaEventParameters> schemaReadyFut =
new CompletableFuture<>();
- CompletableFuture.allOf(schemaReadyFut)
+ schemaReadyFut
.exceptionally(e -> {
LOG.error("Failed to upgrade schema for a table [name=" +
tblName + ", id=" + tblId + ']', e);
- onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName), e);
+ onEvent(TableEvent.ALTER, new TableEventParameters(tblId,
tblName, tbl), e);
Review comment:
```suggestion
onEvent(TableEvent.ALTER, new TableEventParameters(tbl),
e);
```
##########
File path: modules/api/src/main/java/org/apache/ignite/sql/package-info.java
##########
@@ -0,0 +1,21 @@
+/*
+ * 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.
+ */
+
+/**
+ * Contains API classes for sql execution.
Review comment:
Actually this package should include classes and interfaces belonging to
Public API only
--
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]
