You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by GitBox <gi...@apache.org> on 2021/03/25 05:05:54 UTC
[GitHub] [phoenix] tkhurana opened a new pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
tkhurana opened a new pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak merged pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
swaroopak merged pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r602577275
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java
##########
@@ -586,10 +638,11 @@ public boolean hasNext() {
// the tables in the mutations map
if (!sendAll) {
TableRef key = new TableRef(index);
- MultiRowMutationState multiRowMutationState = mutations.remove(key);
+ List<MultiRowMutationState> multiRowMutationState = mutationsMap.remove(key);
if (multiRowMutationState != null) {
final List<Mutation> deleteMutations = Lists.newArrayList();
- generateMutations(key, mutationTimestamp, serverTimestamp, multiRowMutationState, deleteMutations, null);
+ // for index table there will only be 1 mutation batch in the list
Review comment:
@gjacoby126 For indexes, the only time we see an entry in the map is in case of deletes and that too for immutable indexes. So I don't expect a conflicting update unless someone explicitly does a regular upsert and conditional upsert directly on index table which is theoretically possible but highly unlikely.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#issuecomment-806372669
@kadirozde @gokceni @gjacoby126 @jpisaac
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601719128
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java
##########
@@ -404,67 +434,89 @@ public int getNumRows() {
return numRows;
}
+ private MultiRowMutationState getLastMutationBatch(Map<TableRef, List<MultiRowMutationState>> mutations, TableRef tableRef) {
+ List<MultiRowMutationState> mutationBatches = mutations.get(tableRef);
+ if (mutationBatches == null || mutationBatches.isEmpty()) {
+ return null;
+ }
+ return mutationBatches.get(mutationBatches.size() - 1);
+ }
+
private void joinMutationState(TableRef tableRef, MultiRowMutationState srcRows,
- Map<TableRef, MultiRowMutationState> dstMutations) {
+ Map<TableRef, List<MultiRowMutationState>> dstMutations) {
PTable table = tableRef.getTable();
boolean isIndex = table.getType() == PTableType.INDEX;
- boolean incrementRowCount = dstMutations == this.mutations;
- MultiRowMutationState existingRows = dstMutations.put(tableRef, srcRows);
- if (existingRows != null) { // Rows for that table already exist
- // Loop through new rows and replace existing with new
- for (Map.Entry<ImmutableBytesPtr, RowMutationState> rowEntry : srcRows.entrySet()) {
- // Replace existing row with new row
- RowMutationState existingRowMutationState = existingRows.put(rowEntry.getKey(), rowEntry.getValue());
- if (existingRowMutationState != null) {
- Map<PColumn, byte[]> existingValues = existingRowMutationState.getColumnValues();
- if (existingValues != PRow.DELETE_MARKER) {
- Map<PColumn, byte[]> newRow = rowEntry.getValue().getColumnValues();
- // if new row is PRow.DELETE_MARKER, it means delete, and we don't need to merge it with
- // existing row.
- if (newRow != PRow.DELETE_MARKER) {
- // decrement estimated size by the size of the old row
- estimatedSize -= existingRowMutationState.calculateEstimatedSize();
- // Merge existing column values with new column values
- existingRowMutationState.join(rowEntry.getValue());
- // increment estimated size by the size of the new row
- estimatedSize += existingRowMutationState.calculateEstimatedSize();
- // Now that the existing row has been merged with the new row, replace it back
- // again (since it was merged with the new one above).
- existingRows.put(rowEntry.getKey(), existingRowMutationState);
- }
- }
- } else {
- if (incrementRowCount && !isIndex) { // Don't count index rows in row count
- numRows++;
- // increment estimated size by the size of the new row
- estimatedSize += rowEntry.getValue().calculateEstimatedSize();
- }
- }
- }
- // Put the existing one back now that it's merged
- dstMutations.put(tableRef, existingRows);
- } else {
+ boolean incrementRowCount = dstMutations == this.mutationsMap;
+ // we only need to check if the new mutation batch (srcRows) conflicts with the
+ // last mutation batch
Review comment:
Because we only try to merge the new batch with the last one. That is why we only check for conflicts with the last one. All previous batches are not updated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r602376848
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
+
+ // partial update
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES('a',100) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);
Review comment:
I saw createIndex( on line 624 of this function, that is why I was checking.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601688112
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
I think it's correct as counter1 in the row is 0 from line number 627 or did I confuse myself ðŸ’
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gjacoby126 commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
gjacoby126 commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601889798
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/execute/MutationStateTest.java
##########
@@ -17,23 +17,7 @@
*/
package org.apache.phoenix.execute;
-import static org.apache.phoenix.execute.MutationState.joinSortedIntArrays;
-import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.when;
-
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.SQLException;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Properties;
-
+import com.google.common.collect.ImmutableList;
Review comment:
good to use the shaded version of guava for easier porting to 5.x
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java
##########
@@ -404,67 +434,89 @@ public int getNumRows() {
return numRows;
}
+ private MultiRowMutationState getLastMutationBatch(Map<TableRef, List<MultiRowMutationState>> mutations, TableRef tableRef) {
+ List<MultiRowMutationState> mutationBatches = mutations.get(tableRef);
+ if (mutationBatches == null || mutationBatches.isEmpty()) {
+ return null;
+ }
+ return mutationBatches.get(mutationBatches.size() - 1);
+ }
+
private void joinMutationState(TableRef tableRef, MultiRowMutationState srcRows,
- Map<TableRef, MultiRowMutationState> dstMutations) {
+ Map<TableRef, List<MultiRowMutationState>> dstMutations) {
PTable table = tableRef.getTable();
boolean isIndex = table.getType() == PTableType.INDEX;
- boolean incrementRowCount = dstMutations == this.mutations;
- MultiRowMutationState existingRows = dstMutations.put(tableRef, srcRows);
- if (existingRows != null) { // Rows for that table already exist
- // Loop through new rows and replace existing with new
- for (Map.Entry<ImmutableBytesPtr, RowMutationState> rowEntry : srcRows.entrySet()) {
- // Replace existing row with new row
- RowMutationState existingRowMutationState = existingRows.put(rowEntry.getKey(), rowEntry.getValue());
- if (existingRowMutationState != null) {
- Map<PColumn, byte[]> existingValues = existingRowMutationState.getColumnValues();
- if (existingValues != PRow.DELETE_MARKER) {
- Map<PColumn, byte[]> newRow = rowEntry.getValue().getColumnValues();
- // if new row is PRow.DELETE_MARKER, it means delete, and we don't need to merge it with
- // existing row.
- if (newRow != PRow.DELETE_MARKER) {
- // decrement estimated size by the size of the old row
- estimatedSize -= existingRowMutationState.calculateEstimatedSize();
- // Merge existing column values with new column values
- existingRowMutationState.join(rowEntry.getValue());
- // increment estimated size by the size of the new row
- estimatedSize += existingRowMutationState.calculateEstimatedSize();
- // Now that the existing row has been merged with the new row, replace it back
- // again (since it was merged with the new one above).
- existingRows.put(rowEntry.getKey(), existingRowMutationState);
- }
- }
- } else {
- if (incrementRowCount && !isIndex) { // Don't count index rows in row count
- numRows++;
- // increment estimated size by the size of the new row
- estimatedSize += rowEntry.getValue().calculateEstimatedSize();
- }
- }
- }
- // Put the existing one back now that it's merged
- dstMutations.put(tableRef, existingRows);
- } else {
+ boolean incrementRowCount = dstMutations == this.mutationsMap;
+ // we only need to check if the new mutation batch (srcRows) conflicts with the
+ // last mutation batch
+ MultiRowMutationState existingRows = getLastMutationBatch(dstMutations, tableRef);
+
+ if (existingRows == null) { // no rows found for this table
// Size new map at batch size as that's what it'll likely grow to.
MultiRowMutationState newRows = new MultiRowMutationState(connection.getMutateBatchSize());
newRows.putAll(srcRows);
- dstMutations.put(tableRef, newRows);
+ addMutations(dstMutations, tableRef, newRows);
if (incrementRowCount && !isIndex) {
numRows += srcRows.size();
// if we added all the rows from newMutationState we can just increment the
// estimatedSize by newMutationState.estimatedSize
estimatedSize += srcRows.estimatedSize;
}
+ return;
+ }
+
+ // for conflicting rows
+ MultiRowMutationState conflictingRows = new MultiRowMutationState(connection.getMutateBatchSize());
+
+ // Rows for this table already exist, check for conflicts
+ for (Map.Entry<ImmutableBytesPtr, RowMutationState> rowEntry : srcRows.entrySet()) {
+ ImmutableBytesPtr key = rowEntry.getKey();
+ RowMutationState newRowMutationState = rowEntry.getValue();
+ RowMutationState existingRowMutationState = existingRows.get(key);
+ if (existingRowMutationState == null) {
+ existingRows.put(key, newRowMutationState);
+ if (incrementRowCount && !isIndex) { // Don't count index rows in row count
+ numRows++;
+ // increment estimated size by the size of the new row
+ estimatedSize += newRowMutationState.calculateEstimatedSize();
+ }
+ continue;
+ }
+ Map<PColumn, byte[]> existingValues = existingRowMutationState.getColumnValues();
+ Map<PColumn, byte[]> newValues = newRowMutationState.getColumnValues();
+ if (existingValues != PRow.DELETE_MARKER && newValues != PRow.DELETE_MARKER) {
+ // Check if we can merge existing column values with new column values
+ long beforeMerge = existingRowMutationState.calculateEstimatedSize();
Review comment:
nit: maybe beforeMergeSize? This reads like a boolean name to me.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java
##########
@@ -586,10 +638,11 @@ public boolean hasNext() {
// the tables in the mutations map
if (!sendAll) {
TableRef key = new TableRef(index);
- MultiRowMutationState multiRowMutationState = mutations.remove(key);
+ List<MultiRowMutationState> multiRowMutationState = mutationsMap.remove(key);
if (multiRowMutationState != null) {
final List<Mutation> deleteMutations = Lists.newArrayList();
- generateMutations(key, mutationTimestamp, serverTimestamp, multiRowMutationState, deleteMutations, null);
+ // for index table there will only be 1 mutation batch in the list
Review comment:
why only one mutation batch? What if there are conflicting batches for the index?
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/execute/MutationStateTest.java
##########
@@ -210,4 +216,63 @@ public void testPendingMutationsOnDDL() throws Exception {
+ "( id1 UNSIGNED_INT not null primary key," + "appId1 VARCHAR)");
}
}
+
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ conn.createStatement().execute(
+ "create table MUTATION_TEST1" +
+ "( id1 UNSIGNED_INT not null primary key," +
+ "appId1 VARCHAR)");
+ conn.createStatement().execute(
+ "create table MUTATION_TEST2" +
+ "( id2 UNSIGNED_INT not null primary key," +
+ "appId2 VARCHAR)");
+
+ conn.createStatement().execute("upsert into MUTATION_TEST1(id1,appId1) values(111,'app1')");
+ conn.createStatement().execute(
+ "upsert into MUTATION_TEST1(id1,appId1) values(111, 'app1') ON DUPLICATE KEY UPDATE appId1 = null");
+ conn.createStatement().execute("upsert into MUTATION_TEST2(id2,appId2) values(222,'app2')");
+ conn.createStatement().execute(
+ "upsert into MUTATION_TEST2(id2,appId2) values(222,'app2') ON DUPLICATE KEY UPDATE appId2 = null");
+
+ final PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class);
+ MutationState state = pconn.getMutationState();
+ assertEquals(2, state.getNumRows());
+
+ int actualPairs = 0;
+ Iterator<Pair<byte[], List<Mutation>>> mutations = state.toMutations();
Review comment:
A comment here on what you're checking would be helpful.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601708885
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
@gokceni @swaroopak if the key already exists, we ignore the values provided in upsert and evaluate the expression on
the stored value. So in this the increment happens on '0' which was the previously stored value.
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
@gokceni @swaroopak if the key already exists, we ignore the values provided in upsert and evaluate the expression on the stored value. So in this the increment happens on '0' which was the previously stored value.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#issuecomment-806505854
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 17m 20s | 4.x passed |
| +1 :green_heart: | compile | 1m 44s | 4.x passed |
| +1 :green_heart: | checkstyle | 1m 15s | 4.x passed |
| +1 :green_heart: | javadoc | 1m 24s | 4.x passed |
| +0 :ok: | spotbugs | 5m 18s | phoenix-core in 4.x has 941 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 10m 6s | the patch passed |
| +1 :green_heart: | compile | 1m 49s | the patch passed |
| +1 :green_heart: | javac | 1m 49s | the patch passed |
| -1 :x: | checkstyle | 1m 18s | phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 21s | the patch passed |
| +1 :green_heart: | spotbugs | 5m 51s | phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941) |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 223m 26s | phoenix-core in the patch passed. |
| +1 :green_heart: | asflicense | 0m 9s | The patch does not generate ASF License warnings. |
| | | 273m 52s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/1183 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 63d6d7375042 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / ee4ce9f |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/1/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/1/testReport/ |
| Max. process+thread count | 5010 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/1/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] gokceni commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
gokceni commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601614616
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
Should not counter1 value be 3? We are upserting 1 for counter1 and counter1=1+2
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
Review comment:
To see these switched could be nice like row 635 and 634 swapped so that we are sure they are in the same batch and the order doesn't matter in the batch.
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/execute/MutationStateTest.java
##########
@@ -210,4 +216,63 @@ public void testPendingMutationsOnDDL() throws Exception {
+ "( id1 UNSIGNED_INT not null primary key," + "appId1 VARCHAR)");
}
}
+
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ conn.createStatement().execute(
+ "create table MUTATION_TEST1" +
Review comment:
Use generated table names instead.
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
+
+ // partial update
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES('a',100) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);
Review comment:
Also how about adding a check for char type column updates?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/execute/MutationState.java
##########
@@ -404,67 +434,89 @@ public int getNumRows() {
return numRows;
}
+ private MultiRowMutationState getLastMutationBatch(Map<TableRef, List<MultiRowMutationState>> mutations, TableRef tableRef) {
+ List<MultiRowMutationState> mutationBatches = mutations.get(tableRef);
+ if (mutationBatches == null || mutationBatches.isEmpty()) {
+ return null;
+ }
+ return mutationBatches.get(mutationBatches.size() - 1);
+ }
+
private void joinMutationState(TableRef tableRef, MultiRowMutationState srcRows,
- Map<TableRef, MultiRowMutationState> dstMutations) {
+ Map<TableRef, List<MultiRowMutationState>> dstMutations) {
PTable table = tableRef.getTable();
boolean isIndex = table.getType() == PTableType.INDEX;
- boolean incrementRowCount = dstMutations == this.mutations;
- MultiRowMutationState existingRows = dstMutations.put(tableRef, srcRows);
- if (existingRows != null) { // Rows for that table already exist
- // Loop through new rows and replace existing with new
- for (Map.Entry<ImmutableBytesPtr, RowMutationState> rowEntry : srcRows.entrySet()) {
- // Replace existing row with new row
- RowMutationState existingRowMutationState = existingRows.put(rowEntry.getKey(), rowEntry.getValue());
- if (existingRowMutationState != null) {
- Map<PColumn, byte[]> existingValues = existingRowMutationState.getColumnValues();
- if (existingValues != PRow.DELETE_MARKER) {
- Map<PColumn, byte[]> newRow = rowEntry.getValue().getColumnValues();
- // if new row is PRow.DELETE_MARKER, it means delete, and we don't need to merge it with
- // existing row.
- if (newRow != PRow.DELETE_MARKER) {
- // decrement estimated size by the size of the old row
- estimatedSize -= existingRowMutationState.calculateEstimatedSize();
- // Merge existing column values with new column values
- existingRowMutationState.join(rowEntry.getValue());
- // increment estimated size by the size of the new row
- estimatedSize += existingRowMutationState.calculateEstimatedSize();
- // Now that the existing row has been merged with the new row, replace it back
- // again (since it was merged with the new one above).
- existingRows.put(rowEntry.getKey(), existingRowMutationState);
- }
- }
- } else {
- if (incrementRowCount && !isIndex) { // Don't count index rows in row count
- numRows++;
- // increment estimated size by the size of the new row
- estimatedSize += rowEntry.getValue().calculateEstimatedSize();
- }
- }
- }
- // Put the existing one back now that it's merged
- dstMutations.put(tableRef, existingRows);
- } else {
+ boolean incrementRowCount = dstMutations == this.mutationsMap;
+ // we only need to check if the new mutation batch (srcRows) conflicts with the
+ // last mutation batch
Review comment:
why? could you add it to the comment
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
+
+ // partial update
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES('a',100) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);
Review comment:
Recommend adding a check that index row is updated as well
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/execute/MutationStateTest.java
##########
@@ -210,4 +216,63 @@ public void testPendingMutationsOnDDL() throws Exception {
+ "( id1 UNSIGNED_INT not null primary key," + "appId1 VARCHAR)");
}
}
+
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ try (Connection conn = DriverManager.getConnection(getUrl())) {
+ conn.createStatement().execute(
+ "create table MUTATION_TEST1" +
+ "( id1 UNSIGNED_INT not null primary key," +
+ "appId1 VARCHAR)");
+ conn.createStatement().execute(
+ "create table MUTATION_TEST2" +
+ "( id2 UNSIGNED_INT not null primary key," +
+ "appId2 VARCHAR)");
+
+ conn.createStatement().execute("upsert into MUTATION_TEST1(id1,appId1) values(111,'app1')");
+ conn.createStatement().execute(
+ "upsert into MUTATION_TEST1(id1,appId1) values(111, 'app1') ON DUPLICATE KEY UPDATE appId1 = null");
+ conn.createStatement().execute("upsert into MUTATION_TEST2(id2,appId2) values(222,'app2')");
+ conn.createStatement().execute(
+ "upsert into MUTATION_TEST2(id2,appId2) values(222,'app2') ON DUPLICATE KEY UPDATE appId2 = null");
+
+ final PhoenixConnection pconn = conn.unwrap(PhoenixConnection.class);
+ MutationState state = pconn.getMutationState();
+ assertEquals(2, state.getNumRows());
+
+ int actualPairs = 0;
+ Iterator<Pair<byte[], List<Mutation>>> mutations = state.toMutations();
+ while (mutations.hasNext()) {
+ Pair<byte[], List<Mutation>> nextTable = mutations.next();
+ ++actualPairs;
+ assertEquals(1, nextTable.getSecond().size());
+ }
+ assertEquals(4, actualPairs);
+
+ List<Map<TableRef, MultiRowMutationState>> commitBatches = state.createCommitBatches();
+ assertEquals(2, commitBatches.size());
+ // first commit batch should only contain regular upserts
+ verifyCommitBatch(commitBatches.get(0), false);
+ verifyCommitBatch(commitBatches.get(1), true);
+ }
+ }
+
+ private void verifyCommitBatch(Map<TableRef, MultiRowMutationState> commitBatch, boolean conditional) {
+ // one for each table
+ assertEquals(2, commitBatch.size());
Review comment:
recommend adding expected size as parameter
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601729841
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
Cool, that's what I meant with the value from line 627.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#issuecomment-808135772
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 17s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 15m 2s | 4.x passed |
| +1 :green_heart: | compile | 1m 9s | 4.x passed |
| +1 :green_heart: | checkstyle | 0m 43s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 56s | 4.x passed |
| +0 :ok: | spotbugs | 3m 56s | phoenix-core in 4.x has 941 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 8m 8s | the patch passed |
| +1 :green_heart: | compile | 1m 8s | the patch passed |
| +1 :green_heart: | javac | 1m 8s | the patch passed |
| -1 :x: | checkstyle | 0m 55s | phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866) |
| +1 :green_heart: | whitespace | 0m 1s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 1m 0s | the patch passed |
| +1 :green_heart: | spotbugs | 4m 6s | phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941) |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 264m 56s | phoenix-core in the patch passed. |
| +1 :green_heart: | asflicense | 0m 11s | The patch does not generate ASF License warnings. |
| | | 304m 27s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/1183 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux fb0938c38a17 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 3535708 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/2/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/2/testReport/ |
| Max. process+thread count | 4784 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/2/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] stoty commented on pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
stoty commented on pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#issuecomment-813640233
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 5m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
||| _ 4.x Compile Tests _ |
| +1 :green_heart: | mvninstall | 15m 13s | 4.x passed |
| +1 :green_heart: | compile | 1m 3s | 4.x passed |
| +1 :green_heart: | checkstyle | 0m 40s | 4.x passed |
| +1 :green_heart: | javadoc | 0m 52s | 4.x passed |
| +0 :ok: | spotbugs | 3m 13s | phoenix-core in 4.x has 941 extant spotbugs warnings. |
||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 6m 44s | the patch passed |
| +1 :green_heart: | compile | 1m 3s | the patch passed |
| +1 :green_heart: | javac | 1m 3s | the patch passed |
| -1 :x: | checkstyle | 0m 43s | phoenix-core: The patch generated 123 new + 803 unchanged - 63 fixed = 926 total (was 866) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +1 :green_heart: | javadoc | 0m 47s | the patch passed |
| +1 :green_heart: | spotbugs | 3m 27s | phoenix-core generated 0 new + 940 unchanged - 1 fixed = 940 total (was 941) |
||| _ Other Tests _ |
| -1 :x: | unit | 193m 2s | phoenix-core in the patch failed. |
| +1 :green_heart: | asflicense | 0m 36s | The patch does not generate ASF License warnings. |
| | | 235m 40s | |
| Reason | Tests |
|-------:|:------|
| Failed junit tests | phoenix.end2end.index.PartialIndexRebuilderIT |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/phoenix/pull/1183 |
| Optional Tests | dupname asflicense javac javadoc unit spotbugs hbaseanti checkstyle compile |
| uname | Linux 4aee66818618 4.15.0-128-generic #131-Ubuntu SMP Wed Dec 9 06:57:35 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev/phoenix-personality.sh |
| git revision | 4.x / 5c57004 |
| Default Java | Private Build-1.8.0_242-8u242-b08-0ubuntu3~16.04-b08 |
| checkstyle | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/3/artifact/yetus-general-check/output/diff-checkstyle-phoenix-core.txt |
| unit | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/3/artifact/yetus-general-check/output/patch-unit-phoenix-core.txt |
| Test Results | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/3/testReport/ |
| Max. process+thread count | 5304 (vs. ulimit of 30000) |
| modules | C: phoenix-core U: phoenix-core |
| Console output | https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1183/3/console |
| versions | git=2.7.4 maven=3.3.9 spotbugs=4.1.3 |
| Powered by | Apache Yetus 0.12.0 https://yetus.apache.org |
This message was automatically generated.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r602584532
##########
File path: phoenix-core/src/test/java/org/apache/phoenix/execute/MutationStateTest.java
##########
@@ -17,23 +17,7 @@
*/
package org.apache.phoenix.execute;
-import static org.apache.phoenix.execute.MutationState.joinSortedIntArrays;
-import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
-import static org.junit.Assert.assertArrayEquals;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
-import static org.mockito.Mockito.doReturn;
-import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.spy;
-import static org.mockito.Mockito.when;
-
-import java.sql.Connection;
-import java.sql.DriverManager;
-import java.sql.SQLException;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Properties;
-
+import com.google.common.collect.ImmutableList;
Review comment:
@gjacoby126 I opened this PR against apache 4.x. The version on master is already using the shaded version.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r602034647
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
+
+ // partial update
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES('a',100) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);
Review comment:
There is no index defined yet. This change addresses the problem on the data table.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] tkhurana commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
tkhurana commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r602450315
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
+
+ // row exists
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a', 7, 4)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 9, 4);
+
+ // partial update
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES('a',100) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s (pk, counter2) VALUES ('a',125)", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 11, 125);
Review comment:
That is actually local index
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [phoenix] swaroopak commented on a change in pull request #1183: PHOENIX-6420 Wrong result when conditional and regular upserts are passed in the same commit batch
Posted by GitBox <gi...@apache.org>.
swaroopak commented on a change in pull request #1183:
URL: https://github.com/apache/phoenix/pull/1183#discussion_r601688112
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/end2end/OnDuplicateKeyIT.java
##########
@@ -614,6 +614,48 @@ public void testRowsCreatedViaUpsertOnDuplicateKeyShouldNotBeReturnedInQueryIfNo
conn.close();
}
+ @Test
+ public void testOnDupAndUpsertInSameCommitBatch() throws Exception {
+ Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ String tableName = generateUniqueName();
+ try (Connection conn = DriverManager.getConnection(getUrl(), props)) {
+ String ddl = "create table " + tableName + "(pk varchar primary key, counter1 bigint, counter2 smallint)";
+ conn.createStatement().execute(ddl);
+ createIndex(conn, tableName);
+
+ // row doesn't exist
+ conn.createStatement().execute(String.format("UPSERT INTO %s VALUES('a',0,1)", tableName));
+ conn.createStatement().execute(String.format(
+ "UPSERT INTO %s VALUES('a',1,1) ON DUPLICATE KEY UPDATE counter1 = counter1 + 2", tableName));
+ conn.commit();
+ assertRow(conn, tableName, "a", 2, 1);
Review comment:
I think it's correct as counter1 in the row is 0 from line number 627. 🤔
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org