PakhomovAlexander commented on code in PR #3267: URL: https://github.com/apache/ignite-3/pull/3267#discussion_r1504119571
########## modules/api/src/main/java/org/apache/ignite/catalog/annotations/Column.java: ########## @@ -0,0 +1,74 @@ +/* + * 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.catalog.annotations; + +import static java.lang.annotation.ElementType.FIELD; +import static java.lang.annotation.ElementType.METHOD; +import static java.lang.annotation.RetentionPolicy.RUNTIME; + +import java.lang.annotation.Retention; +import java.lang.annotation.Target; + +/** + * Describes a column of the table. Type of the column is derived from the type of the annotated field or method. + */ +@Target({METHOD, FIELD}) +@Retention(RUNTIME) +public @interface Column { Review Comment: Why do we have Col and Column? ########## modules/api/src/main/java/org/apache/ignite/catalog/Options.java: ########## @@ -0,0 +1,116 @@ +/* + * 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.catalog; + +/** + * SQL query generation options. + */ +public class Options { + private boolean prettyPrint = false; + + private boolean quoteIdentifiers = false; + + private int indentWidth = 2; + + /** + * Constructs an options object with default values - prints queries without line breaks, doesn't quote identifiers. + * + * @return Options object. + */ + public static Options defaultOptions() { + return new Options(); + } + + /** + * Returns pretty printing status. + * + * @return {@code true} if pretty printing is enabled. + */ + public boolean isPrettyPrint() { + return prettyPrint; + } + + /** + * Enables pretty printing. + * + * @return Options object. + */ + public Options prettyPrint() { + return prettyPrint(true); + } + + /** + * Sets pretty printing. + * + * @param b If {@code true}, pretty printing is enabled. + * @return Options object. + */ + public Options prettyPrint(boolean b) { + this.prettyPrint = b; + return this; + } + + /** + * Returns quoting identifiers status. + * + * @return {@code true} if identifiers are quoted. + */ + public boolean isQuoteIdentifiers() { + return quoteIdentifiers; + } + + /** + * Enables quoting identifiers. + * + * @return Options object. + */ + public Options quoteIdentifiers() { + return quoteIdentifiers(true); + } + + /** + * Sets quoting identifiers. + * + * @param b If {@code true}, quotes identifiers. + * @return Options object. + */ + public Options quoteIdentifiers(boolean b) { + this.quoteIdentifiers = b; + return this; + } + + /** + * Returns indentation width. + * + * @return Indentation width. + */ + public int indentWidth() { Review Comment: What I don't understand: Why do we have some builder setter methods that return on Options but some are record-style getters? Isn't it confusing? ```java Options opt = Options.defaultOptions().quoteIdentifiers(); // compile Options opt = Options.defaultOptions().indentWidth(); // do not compile ``` I understand a formal reason for that but I would think about another API that would be more straight forward. ########## modules/api/src/main/java/org/apache/ignite/catalog/IgniteCatalog.java: ########## @@ -0,0 +1,56 @@ +/* + * 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.catalog; + +/** + * Provides the ability to create and execute SQL DDL queries from annotated classes or from fluent-style builders. + */ +public interface IgniteCatalog { + /** + * Creates a query object from the annotated record class. + * + * @param recCls Annotated record class. + * @return query object + */ + Query create(Class<?> recCls); Review Comment: I think this is a good place for the JavaDoc with examples how to annotate a class. I, as a user, would be placed with it. ########## modules/catalog-dsl/src/main/java/org/apache/ignite/internal/catalog/sql/Name.java: ########## @@ -0,0 +1,48 @@ +/* + * 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; + +/** + * Qualified SQL identifier. Review Comment: Quoted? ########## modules/api/src/main/java/org/apache/ignite/catalog/ColumnSorted.java: ########## @@ -0,0 +1,127 @@ +/* + * 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.catalog; + +import java.util.Objects; + +/** + * Column reference with sort order, used in indexes or primary keys. + */ +public class ColumnSorted { Review Comment: Shouldn't it be a final class? ########## modules/catalog-dsl/build.gradle: ########## @@ -0,0 +1,35 @@ +/* + * 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. + */ + +apply from: "$rootDir/buildscripts/java-core.gradle" +apply from: "$rootDir/buildscripts/publishing.gradle" +apply from: "$rootDir/buildscripts/java-junit5.gradle" +apply from: "$rootDir/buildscripts/java-integration-test.gradle" + +description = 'ignite-catalog-ddl' Review Comment: dsl or ddl? ########## modules/catalog-dsl/src/test/java/org/apache/ignite/internal/catalog/sql/CreateTableTest.java: ########## @@ -0,0 +1,188 @@ +/* + * 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.catalog.ColumnType.INTEGER; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.is; + +import org.apache.ignite.catalog.IndexType; +import org.apache.ignite.catalog.Options; +import org.junit.jupiter.api.Test; + +class CreateTableTest { + @Test + void ifNotExists() { + String sql = createTable().ifNotExists().name("table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE IF NOT EXISTS table1 (col1 int);")); + + // quote identifiers + sql = createTableQuoted().ifNotExists().name("table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE IF NOT EXISTS \"table1\" (\"col1\" int);")); + } + + @Test + void names() { + String sql = createTable().name("public", "table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE public.table1 (col1 int);")); + + sql = createTable().name("", "table;1--test\n\r\t;").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE table1 (col1 int);")); + + // quote identifiers + sql = createTableQuoted().name("public", "table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE \"public\".\"table1\" (\"col1\" int);")); + + sql = createTableQuoted().name("", "table;1--test\n\r\t;").addColumn("col1", INTEGER).toSqlString(); Review Comment: And here it is even more interesting. ########## modules/catalog-dsl/src/main/java/org/apache/ignite/internal/catalog/sql/CreateFromAnnotationsImpl.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.catalog.ColumnSorted.column; +import static org.apache.ignite.internal.catalog.sql.QueryUtils.mapArrayToList; +import static org.apache.ignite.table.mapper.Mapper.nativelySupported; + +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.List; +import org.apache.ignite.catalog.ColumnSorted; +import org.apache.ignite.catalog.ColumnType; +import org.apache.ignite.catalog.DefaultZone; +import org.apache.ignite.catalog.IndexType; +import org.apache.ignite.catalog.Options; +import org.apache.ignite.catalog.annotations.Col; +import org.apache.ignite.catalog.annotations.Column; +import org.apache.ignite.catalog.annotations.Id; +import org.apache.ignite.catalog.annotations.Index; +import org.apache.ignite.catalog.annotations.Table; +import org.apache.ignite.catalog.annotations.Zone; +import org.apache.ignite.sql.IgniteSql; + +class CreateFromAnnotationsImpl extends AbstractCatalogQuery { + private CreateZoneImpl createZone; + + private CreateTableImpl createTable; + + private IndexType pkType; + + CreateFromAnnotationsImpl(IgniteSql sql, Options options) { + super(sql, options); + } + + CreateFromAnnotationsImpl keyValueView(Class<?> keyClass, Class<?> valueClass) { Review Comment: Why is the method called ..View? It has nothing in common with a view. ########## modules/catalog-dsl/src/main/java/org/apache/ignite/internal/catalog/sql/CreateFromAnnotationsImpl.java: ########## @@ -0,0 +1,195 @@ +/* + * 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.catalog.ColumnSorted.column; +import static org.apache.ignite.internal.catalog.sql.QueryUtils.mapArrayToList; +import static org.apache.ignite.table.mapper.Mapper.nativelySupported; + +import java.lang.reflect.Field; +import java.lang.reflect.Modifier; +import java.util.ArrayList; +import java.util.List; +import org.apache.ignite.catalog.ColumnSorted; +import org.apache.ignite.catalog.ColumnType; +import org.apache.ignite.catalog.DefaultZone; +import org.apache.ignite.catalog.IndexType; +import org.apache.ignite.catalog.Options; +import org.apache.ignite.catalog.annotations.Col; +import org.apache.ignite.catalog.annotations.Column; +import org.apache.ignite.catalog.annotations.Id; +import org.apache.ignite.catalog.annotations.Index; +import org.apache.ignite.catalog.annotations.Table; +import org.apache.ignite.catalog.annotations.Zone; +import org.apache.ignite.sql.IgniteSql; + +class CreateFromAnnotationsImpl extends AbstractCatalogQuery { + private CreateZoneImpl createZone; + + private CreateTableImpl createTable; + + private IndexType pkType; + + CreateFromAnnotationsImpl(IgniteSql sql, Options options) { + super(sql, options); + } + + CreateFromAnnotationsImpl keyValueView(Class<?> keyClass, Class<?> valueClass) { + if (keyClass.getAnnotation(Table.class) == null && valueClass.getAnnotation(Table.class) == null) { + throw new IllegalArgumentException("Table annotation must be present."); Review Comment: I think a user-facing error message should be as clear as possible. Here and everywhere else I would expect something like: "Can not find @Table annotation on keyCless.name() or valueClass.name. One of these classes must be annotated with @Table ...". ########## modules/catalog-dsl/src/test/java/org/apache/ignite/internal/catalog/sql/CreateTableTest.java: ########## @@ -0,0 +1,188 @@ +/* + * 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.catalog.ColumnType.INTEGER; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.is; + +import org.apache.ignite.catalog.IndexType; +import org.apache.ignite.catalog.Options; +import org.junit.jupiter.api.Test; + +class CreateTableTest { + @Test + void ifNotExists() { + String sql = createTable().ifNotExists().name("table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE IF NOT EXISTS table1 (col1 int);")); + + // quote identifiers + sql = createTableQuoted().ifNotExists().name("table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE IF NOT EXISTS \"table1\" (\"col1\" int);")); + } + + @Test + void names() { + String sql = createTable().name("public", "table1").addColumn("col1", INTEGER).toSqlString(); + assertThat(sql, is("CREATE TABLE public.table1 (col1 int);")); + + sql = createTable().name("", "table;1--test\n\r\t;").addColumn("col1", INTEGER).toSqlString(); Review Comment: Thaaaat is reeeeally interesting. How do we define that "table;1--test\n\r\t;" will became "table1"??? -- 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]
