zstan commented on code in PR #13366: URL: https://github.com/apache/ignite/pull/13366#discussion_r3756278920
########## modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/sql/SqlSelectForUpdateParserTest.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.processors.query.calcite.sql; + +import org.apache.calcite.sql.SqlExplain; +import org.apache.calcite.sql.SqlIdentifier; +import org.apache.calcite.sql.SqlNode; +import org.apache.calcite.sql.SqlNodeList; +import org.apache.calcite.sql.SqlSelect; +import org.apache.calcite.sql.parser.SqlParseException; +import org.apache.calcite.sql.parser.SqlParser; +import org.apache.ignite.internal.processors.query.calcite.sql.generated.IgniteSqlParserImpl; +import org.apache.ignite.testframework.GridTestUtils; +import org.apache.ignite.testframework.junits.common.GridCommonAbstractTest; +import org.junit.Test; + +import static org.hamcrest.CoreMatchers.instanceOf; +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.nullValue; +import static org.hamcrest.MatcherAssert.assertThat; + +/** Tests for parsing {@code SELECT ... FOR UPDATE} syntax. */ +public class SqlSelectForUpdateParserTest extends GridCommonAbstractTest { + /** Regular SELECT is returned as-is (no wrapping). */ + @Test + public void regularSelectIsNotWrapped() throws SqlParseException { + SqlNode node = parse("SELECT name FROM Person"); + + assertThat(node, instanceOf(SqlSelect.class)); + } + + /** Minimal FOR UPDATE with no options. */ + @Test + public void forUpdateNoOptions() throws SqlParseException { + SqlNode node = parse("SELECT name FROM Person FOR UPDATE"); Review Comment: Can we parse unexpected queries a bit earlier ? I see this validation in PrepareServiceImpl: ``` if (from == null) throw new IgniteSQLException( "SELECT FOR UPDATE requires a FROM clause", IgniteQueryErrorCode.UNSUPPORTED_OPERATION); ``` can it be moved into : IgniteSqlSelectForUpdate constructor ? ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); + + collectTableRefs(from, ctx.schemaName(), tableRefs); + validateSelectForUpdateTables(tableRefs, ctx); + + SqlNodeList origList = select.getSelectList(); + SqlNodeList newList = new SqlNodeList(SqlParserPos.ZERO); + + for (SqlNode col : origList) + newList.add(col); Review Comment: ```suggestion newList.addAll(origList); ``` ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); + + collectTableRefs(from, ctx.schemaName(), tableRefs); + validateSelectForUpdateTables(tableRefs, ctx); + + SqlNodeList origList = select.getSelectList(); + SqlNodeList newList = new SqlNodeList(SqlParserPos.ZERO); + + for (SqlNode col : origList) + newList.add(col); + + SqlNodeList ofList = forUpdate.ofList(); + int ofColumnCnt = ofList == null ? 0 : ofList.size(); + + if (ofList != null) { + for (SqlNode col : ofList) + newList.add(col); + } + + for (TableRef tableRef : tableRefs) { + newList.add(tableRef.column(QueryUtils.KEY_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VAL_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VER_FIELD_NAME)); + } + + SqlSelect modifiedSelect = new SqlSelect( + SqlParserPos.ZERO, + null, + newList, + select.getFrom(), + select.getWhere(), + select.getGroup(), + select.getHaving(), + select.getWindowList(), + select.getQualify(), + orderList, + offset, + fetch, + select.getHints() + ); + + MultiStepQueryPlan innerPlan = (MultiStepQueryPlan)prepareQuery(modifiedSelect, ctx); + + int lockColumnCnt = tableRefs.size() * 3; Review Comment: ```suggestion int lockSpecificColumnCnt = tableRefs.size() * 3; ``` ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); Review Comment: Question about tables ordering, do we reaaly need it here ? At first look we can simplify it like : ``` Set<TableRef> tableRefs = new HashSet<>(); collectTableRefs(from, ctx.schemaName(), tableRefs); validateSelectForUpdateTables(tableRefs, ctx); ``` all tests are passed for me ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); + + collectTableRefs(from, ctx.schemaName(), tableRefs); + validateSelectForUpdateTables(tableRefs, ctx); + + SqlNodeList origList = select.getSelectList(); + SqlNodeList newList = new SqlNodeList(SqlParserPos.ZERO); + + for (SqlNode col : origList) + newList.add(col); + + SqlNodeList ofList = forUpdate.ofList(); + int ofColumnCnt = ofList == null ? 0 : ofList.size(); + + if (ofList != null) { + for (SqlNode col : ofList) + newList.add(col); Review Comment: ```suggestion newList.addAll(ofList); ``` ########## modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/AbstractBasicIntegrationTest.java: ########## @@ -269,20 +271,34 @@ protected List<List<?>> sqlAsRoot(IgniteEx ignite, String sql) throws Exception /** */ protected List<List<?>> sql(IgniteEx ignite, String sql, Object... params) { - // {@code sql} can contain more than one query. - List<FieldsQueryCursor<List<?>>> allCurs = queryProcessor(ignite).query(queryContext(), "PUBLIC", sql, params); + Transaction tx = ignite.transactions().tx(); + QueryContext ctx = tx == null + ? queryContext() + : QueryContext.of(queryContext(), ((TransactionProxyImpl<?, ?>)tx).tx().xidVersion()); - if (allCurs.size() > 1) { - log.warning("The query statement '" + sql + "' contains " + allCurs.size() + " actual queries. " + - "All the cursors are fetched, but only the last result is returned."); - } + if (tx != null) Review Comment: I also qoubt about axplicit call for 'tx.suspend' here, this is a common function why it need to be called for all test cases can you clarify ? ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); + + collectTableRefs(from, ctx.schemaName(), tableRefs); + validateSelectForUpdateTables(tableRefs, ctx); + + SqlNodeList origList = select.getSelectList(); + SqlNodeList newList = new SqlNodeList(SqlParserPos.ZERO); + + for (SqlNode col : origList) + newList.add(col); + + SqlNodeList ofList = forUpdate.ofList(); + int ofColumnCnt = ofList == null ? 0 : ofList.size(); + + if (ofList != null) { + for (SqlNode col : ofList) + newList.add(col); + } + + for (TableRef tableRef : tableRefs) { + newList.add(tableRef.column(QueryUtils.KEY_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VAL_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VER_FIELD_NAME)); + } + + SqlSelect modifiedSelect = new SqlSelect( + SqlParserPos.ZERO, + null, + newList, + select.getFrom(), + select.getWhere(), + select.getGroup(), + select.getHaving(), + select.getWindowList(), + select.getQualify(), + orderList, + offset, + fetch, + select.getHints() + ); + + MultiStepQueryPlan innerPlan = (MultiStepQueryPlan)prepareQuery(modifiedSelect, ctx); + + int lockColumnCnt = tableRefs.size() * 3; + int userColCnt = innerPlan.fieldsMetadata().rowType().getFieldCount() - ofColumnCnt - lockColumnCnt; Review Comment: ```suggestion int requestedColCnt = innerPlan.fieldsMetadata().rowType().getFieldCount() - ofColumnCnt - lockColumnCnt; ``` ########## modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/PrepareServiceImpl.java: ########## @@ -136,6 +154,311 @@ public PrepareServiceImpl(GridKernalContext ctx) { } } + /** + * Prepares a {@code SELECT ... FOR UPDATE} statement. + * + * <p>Steps: + * <ol> + * <li>Unwrap optional ORDER BY and validate that the inner query is a plain {@link SqlSelect} + * (not UNION etc.).</li> + * <li>Collect and validate cache-based tables from the FROM clause.</li> + * <li>Append OF columns for validation and lock columns for every table.</li> + * <li>Prepare the modified SELECT as a normal {@link MultiStepQueryPlan}.</li> + * <li>Resolve tables selected by OF using aliases and validated column origins.</li> + * <li>Return a {@link SelectForUpdatePlan} wrapping the inner plan.</li> + * </ol> + */ + private SelectForUpdatePlan prepareSelectForUpdate(IgniteSqlSelectForUpdate forUpdate, PlanningContext ctx) + throws ValidationException { + SqlNode innerQry = forUpdate.query(); + + SqlOrderBy orderBy = null; + + if (innerQry instanceof SqlOrderBy) { + orderBy = (SqlOrderBy)innerQry; + innerQry = orderBy.query; + } + + if (!(innerQry instanceof SqlSelect)) + throw new IgniteSQLException( + "SELECT FOR UPDATE is only supported for plain SELECT statements", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + SqlSelect select = (SqlSelect)innerQry; + SqlNodeList orderList = orderBy == null ? select.getOrderList() : orderBy.orderList; + SqlNode offset = orderBy == null ? select.getOffset() : orderBy.offset; + SqlNode fetch = orderBy == null ? select.getFetch() : orderBy.fetch; + + validateSelectForUpdateShape(select, orderList, ctx.planner()); + + // Unwrap optional AS alias around the table reference. + SqlNode from = select.getFrom(); + + if (from == null) + throw new IgniteSQLException( + "SELECT FOR UPDATE requires a FROM clause", + IgniteQueryErrorCode.UNSUPPORTED_OPERATION); + + List<TableRef> tableRefs = new ArrayList<>(); + + collectTableRefs(from, ctx.schemaName(), tableRefs); + validateSelectForUpdateTables(tableRefs, ctx); + + SqlNodeList origList = select.getSelectList(); + SqlNodeList newList = new SqlNodeList(SqlParserPos.ZERO); + + for (SqlNode col : origList) + newList.add(col); + + SqlNodeList ofList = forUpdate.ofList(); + int ofColumnCnt = ofList == null ? 0 : ofList.size(); + + if (ofList != null) { + for (SqlNode col : ofList) + newList.add(col); + } + + for (TableRef tableRef : tableRefs) { + newList.add(tableRef.column(QueryUtils.KEY_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VAL_FIELD_NAME)); + newList.add(tableRef.column(QueryUtils.VER_FIELD_NAME)); + } + + SqlSelect modifiedSelect = new SqlSelect( + SqlParserPos.ZERO, + null, + newList, + select.getFrom(), + select.getWhere(), + select.getGroup(), + select.getHaving(), + select.getWindowList(), + select.getQualify(), + orderList, + offset, + fetch, + select.getHints() + ); + + MultiStepQueryPlan innerPlan = (MultiStepQueryPlan)prepareQuery(modifiedSelect, ctx); + + int lockColumnCnt = tableRefs.size() * 3; Review Comment: can you comment here that 3 is because of _KEY + _VAL + VER ? -- 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]
