stoty commented on code in PR #2222: URL: https://github.com/apache/phoenix/pull/2222#discussion_r2206523514
########## phoenix-core/src/it/java/org/apache/phoenix/end2end/MultipleUpsertIT.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.phoenix.end2end; + +import org.apache.phoenix.util.SchemaUtil; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.Properties; + +import static org.apache.phoenix.query.QueryConstants.DEFAULT_COLUMN_FAMILY; +import static org.apache.phoenix.query.QueryConstants.ENCODED_CQ_COUNTER_INITIAL_VALUE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +@Category(ParallelStatsDisabledTest.class) +public class MultipleUpsertIT extends ParallelStatsDisabledIT { + @Test + public void testUpsertMultiple() throws Exception { + Properties props = new Properties(); + Connection conn = DriverManager.getConnection(getUrl(), props); + String tableName = generateUniqueName(); + String ddl = "CREATE TABLE " + tableName + "(K VARCHAR NOT NULL PRIMARY KEY, INT INTEGER, INT2 INTEGER)"; + conn.createStatement().execute(ddl); + + + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('A', 11, 12)"); + conn.createStatement().execute("UPSERT INTO " + tableName + "(K, INT) VALUES ('B', 2)"); + conn.createStatement().execute("UPSERT INTO " + tableName + "(K, INT, INT2) VALUES ('E', 5, 5),('F', 61, 6)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('C', 31, 32),('D', 41, 42)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('G', 7, 72),('H', 8)"); + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('I', 9),('I', 10)"); Review Comment: I don't think this is valid standard SQL (i.e. not all columns are specified, but no column list either) , but if this works for the single value upsert then it's fine. ########## phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java: ########## @@ -1286,75 +1302,83 @@ public MutationState execute() throws SQLException { ImmutableBytesWritable ptr = context.getTempPtr(); final SequenceManager sequenceManager = context.getSequenceManager(); // Next evaluate all the expressions - int nodeIndex = nodeIndexOffset; PTable table = tableRef.getTable(); Tuple tuple = sequenceManager.getSequenceCount() == 0 ? null : sequenceManager.newSequenceTuple(null); - for (Expression constantExpression : constantExpressions) { - if (!constantExpression.isStateless()) { - nodeIndex++; - continue; - } - PColumn column = allColumns.get(columnIndexes[nodeIndex]); - constantExpression.evaluate(tuple, ptr); - Object value = null; - if (constantExpression.getDataType() != null) { - value = constantExpression.getDataType().toObject(ptr, constantExpression.getSortOrder(), - constantExpression.getMaxLength(), constantExpression.getScale()); - if (!constantExpression.getDataType().isCoercibleTo(column.getDataType(), value)) { - throw TypeMismatchException.newException( - constantExpression.getDataType(), column.getDataType(), "expression: " - + constantExpression.toString() + " in column " + column); + + int index = -1; + for (byte[][] valuesListIem : valuesList) { Review Comment: Also a typo in valuesListIem ########## phoenix-core/src/test/java/org/apache/phoenix/parse/QueryParserTest.java: ########## @@ -803,6 +803,26 @@ public void testValidUpsertSelectHint() throws Exception { parseQuery(sql); } + @Test + public void testValidMultipleUpsert() throws Exception { + String sql = ( + ( + "upsert into t VALUES(1,2),(3,4)")); + parseQuery(sql); + } + + @Test + public void testValidMultipleUpsert2() throws Exception { + String sql = "upsert into t(a,b) VALUES(1,2),(3,4)"; + parseQuery(sql); + } + + @Test + public void testValidMultipleUpsert3() throws Exception { Review Comment: A few more failure cases to exercise the parser would be nice. i.e. no value list at all, no commas between, not closing some parentheses and similar. ########## phoenix-core/src/it/java/org/apache/phoenix/end2end/MultipleUpsertIT.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.phoenix.end2end; + +import org.apache.phoenix.util.SchemaUtil; +import org.junit.Test; +import org.junit.experimental.categories.Category; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.Statement; +import java.util.Properties; + +import static org.apache.phoenix.query.QueryConstants.DEFAULT_COLUMN_FAMILY; +import static org.apache.phoenix.query.QueryConstants.ENCODED_CQ_COUNTER_INITIAL_VALUE; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +@Category(ParallelStatsDisabledTest.class) +public class MultipleUpsertIT extends ParallelStatsDisabledIT { + @Test + public void testUpsertMultiple() throws Exception { + Properties props = new Properties(); + Connection conn = DriverManager.getConnection(getUrl(), props); + String tableName = generateUniqueName(); + String ddl = "CREATE TABLE " + tableName + "(K VARCHAR NOT NULL PRIMARY KEY, INT INTEGER, INT2 INTEGER)"; + conn.createStatement().execute(ddl); + + + conn.createStatement().execute("UPSERT INTO " + tableName + " VALUES ('A', 11, 12)"); Review Comment: I'd like to also have some tests for expressions and mixed expressions and literals. i.e. VALUES (substring('APPLE',0,1), 2*2) ########## phoenix-core-client/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java: ########## @@ -1286,75 +1302,83 @@ public MutationState execute() throws SQLException { ImmutableBytesWritable ptr = context.getTempPtr(); final SequenceManager sequenceManager = context.getSequenceManager(); // Next evaluate all the expressions - int nodeIndex = nodeIndexOffset; PTable table = tableRef.getTable(); Tuple tuple = sequenceManager.getSequenceCount() == 0 ? null : sequenceManager.newSequenceTuple(null); - for (Expression constantExpression : constantExpressions) { - if (!constantExpression.isStateless()) { - nodeIndex++; - continue; - } - PColumn column = allColumns.get(columnIndexes[nodeIndex]); - constantExpression.evaluate(tuple, ptr); - Object value = null; - if (constantExpression.getDataType() != null) { - value = constantExpression.getDataType().toObject(ptr, constantExpression.getSortOrder(), - constantExpression.getMaxLength(), constantExpression.getScale()); - if (!constantExpression.getDataType().isCoercibleTo(column.getDataType(), value)) { - throw TypeMismatchException.newException( - constantExpression.getDataType(), column.getDataType(), "expression: " - + constantExpression.toString() + " in column " + column); + + int index = -1; + for (byte[][] valuesListIem : valuesList) { Review Comment: Keeping the two lists in sync like this awkware. Using an old-style for() would be more readable to me: for(int index=0; index<valuesList.length();i++) and then use the index to get both the valuesListItem and constantExpression. -- 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: issues-unsubscr...@phoenix.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
