tpalfy commented on code in PR #9640: URL: https://github.com/apache/nifi/pull/9640#discussion_r1923933838
########## nifi-extension-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/db/impl/DatabaseAdapterDatabaseDialectService.java: ########## @@ -0,0 +1,217 @@ +/* + * 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.nifi.processors.standard.db.impl; + +import org.apache.nifi.controller.AbstractControllerService; +import org.apache.nifi.database.dialect.service.api.ColumnDefinition; +import org.apache.nifi.database.dialect.service.api.DatabaseDialectService; +import org.apache.nifi.database.dialect.service.api.PageRequest; +import org.apache.nifi.database.dialect.service.api.QueryClause; +import org.apache.nifi.database.dialect.service.api.QueryClauseType; +import org.apache.nifi.database.dialect.service.api.QueryStatementRequest; +import org.apache.nifi.database.dialect.service.api.StandardStatementResponse; +import org.apache.nifi.database.dialect.service.api.StatementRequest; +import org.apache.nifi.database.dialect.service.api.StatementResponse; +import org.apache.nifi.database.dialect.service.api.StatementType; +import org.apache.nifi.database.dialect.service.api.TableDefinition; +import org.apache.nifi.processors.standard.db.ColumnDescription; +import org.apache.nifi.processors.standard.db.DatabaseAdapter; +import org.apache.nifi.processors.standard.db.DatabaseAdapterDescriptor; +import org.apache.nifi.processors.standard.db.TableSchema; + +import java.util.Collections; +import java.util.Iterator; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; +import java.util.stream.Collectors; + +/** + * Transitional implementation of Database Dialect Service bridging to existing Database Adapters Review Comment: It wasn't clear that this is an irregular ControllerService in a sense that it doesn't have a framework-managed lifecycle. Could be useful to add this technical information to the doc. ########## nifi-extension-bundles/nifi-standard-services/nifi-database-dialect-service-bundle/nifi-database-dialect-service/src/main/java/org/apache/nifi/database/dialect/service/StandardDatabaseDialectService.java: ########## @@ -0,0 +1,326 @@ +/* + * 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.nifi.database.dialect.service; + +import org.apache.nifi.annotation.documentation.CapabilityDescription; +import org.apache.nifi.annotation.documentation.Tags; +import org.apache.nifi.controller.AbstractControllerService; +import org.apache.nifi.database.dialect.service.api.ColumnDefinition; +import org.apache.nifi.database.dialect.service.api.DatabaseDialectService; +import org.apache.nifi.database.dialect.service.api.PageRequest; +import org.apache.nifi.database.dialect.service.api.QueryClause; +import org.apache.nifi.database.dialect.service.api.QueryClauseType; +import org.apache.nifi.database.dialect.service.api.QueryStatementRequest; +import org.apache.nifi.database.dialect.service.api.StandardStatementResponse; +import org.apache.nifi.database.dialect.service.api.StatementRequest; +import org.apache.nifi.database.dialect.service.api.StatementResponse; +import org.apache.nifi.database.dialect.service.api.StatementType; +import org.apache.nifi.database.dialect.service.api.TableDefinition; + +import java.sql.JDBCType; +import java.util.Iterator; +import java.util.List; +import java.util.Objects; +import java.util.Optional; +import java.util.OptionalLong; +import java.util.Set; + +@CapabilityDescription(""" + Database Dialect Service supporting ANSI SQL. + Supported Statement Types: ALTER, CREATE, SELECT +""" +) +@Tags({ "Relational", "Database", "JDBC", "SQL" }) +public class StandardDatabaseDialectService extends AbstractControllerService implements DatabaseDialectService { Review Comment: Just a thought for consideration: This DialectService is similar in concept to the GenericDatabaseAdapter - could be easier to get the whole picture with the name GenericDatabaseDialectService ########## nifi-extension-bundles/nifi-standard-services/nifi-database-dialect-service-api/src/main/java/org/apache/nifi/database/dialect/service/api/ColumnDefinition.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.nifi.database.dialect.service.api; + +import java.util.Optional; + +/** + * Database Table Column Definition + */ +public interface ColumnDefinition { + String columnName(); + + int dataType(); + + Nullable nullable(); + + Optional<String> defaultValue(); Review Comment: The `defaultValue()` doesn't seem to be called from anywhere. Is it intentional? ########## nifi-extension-bundles/nifi-standard-services/nifi-database-dialect-service-api/src/main/java/org/apache/nifi/database/dialect/service/api/QueryStatementRequest.java: ########## @@ -0,0 +1,31 @@ +/* + * 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.nifi.database.dialect.service.api; + +import java.util.Collection; +import java.util.Optional; + +/** + * Query extension of Statement Request with additional properties for SELECT statements + */ +public interface QueryStatementRequest extends StatementRequest { + Optional<String> derivedTable(); + + Collection<QueryClause> queryClauses(); Review Comment: This is a bit confusing. If I understand correctly, there can be 0 or 1 WHERE and 0 or 1 ORDER BY clause. When the values for those are read, the same logic is used in StandardDatabaseDialectService and in DatabaseAdapterDatabaseDialectService: ```java final Optional<QueryClause> whereQueryClause = queryStatementRequest.queryClauses().stream() .filter(queryClause -> QueryClauseType.WHERE == queryClause.queryClauseType()) .findFirst(); final Optional<QueryClause> orderByQueryClause = queryStatementRequest.queryClauses().stream() .filter(queryClause -> QueryClauseType.ORDER_BY == queryClause.queryClauseType()) .findFirst(); ``` Is there a reason why don't have a more explicit and strongly typed approach, with an Optional<WhereClause> and an Optional<OrderByClause>? This can actually lead to an exception. See my comment on `DatabaseParameterProvider`. ########## nifi-extension-bundles/nifi-standard-bundle/nifi-standard-parameter-providers/src/main/java/org/apache/nifi/parameter/DatabaseParameterProvider.java: ########## @@ -233,8 +223,23 @@ private void validateValueNotNull(final String value, final String columnName) { } String getQuery(final ConfigurationContext context, final String tableName, final List<String> columns, final String whereClause) { - final DatabaseAdapter dbAdapter = dbAdapters.get(context.getProperty(DB_TYPE).getValue()); - return dbAdapter.getSelectStatement(tableName, StringUtils.join(columns, ", "), whereClause, null, null, null); + final String databaseType = context.getProperty(DB_TYPE).getValue(); + final DatabaseDialectService databaseDialectService = DatabaseAdapterDescriptor.getDatabaseDialectService(context, DATABASE_DIALECT_SERVICE, databaseType); + + final List<ColumnDefinition> columnDefinitions = columns.stream() + .map(StandardColumnDefinition::new) + .map(ColumnDefinition.class::cast) + .toList(); + final TableDefinition tableDefinition = new TableDefinition(Optional.empty(), Optional.empty(), tableName, columnDefinitions); + final QueryStatementRequest queryStatementRequest = new StandardQueryStatementRequest( + StatementType.SELECT, + tableDefinition, + Optional.empty(), + List.of(new QueryClause(QueryClauseType.WHERE, whereClause)), + Optional.empty() + ); + final StatementResponse statementResponse = databaseDialectService.getStatement(queryStatementRequest); Review Comment: When the optional WHERE property is not set (null), the generated select statement will have a "WHERE null" clause, resulting to an exception (at least it did when testing on Oracle). ########## nifi-extension-bundles/nifi-standard-services/nifi-database-dialect-service-api/src/main/java/org/apache/nifi/database/dialect/service/api/StandardQueryStatementRequest.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.nifi.database.dialect.service.api; + +import java.util.Collection; +import java.util.List; +import java.util.Optional; + +/** + * Standard implementation of Query Statement Request with required properties + * + * @param statementType SQL Statement Type + * @param tableDefinition Database Table Definition + * @param derivedTable Derived Table Query or empty when not defined + * @param queryClauses Collection of Query Clauses can be empty + * @param pageRequest Page Request can be empty + */ +public record StandardQueryStatementRequest( + StatementType statementType, Review Comment: Can the StatementType anything else but SELECT for a QueryStatementRequest? -- 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]
