From 6e35c0752b1337846bddf427032e3878c9d0c3ab Mon Sep 17 00:00:00 2001 From: Alexander Paschenko Date: Sun, 27 Nov 2016 20:52:50 +0300 Subject: [PATCH 1/3] Renames and duplicate code removal (trivial) --- .../query/h2/DmlStatementsProcessor.java | 136 ++++++++---------- .../query/h2/dml/UpdatePlanBuilder.java | 16 +-- 2 files changed, 66 insertions(+), 86 deletions(-) diff --git a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java index 469e36c0c5fb2..67e1f430235f7 100644 --- a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java +++ b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java @@ -464,22 +464,6 @@ private UpdateResult doUpdate(UpdatePlan plan, QueryCursorImpl> cursor, long res = 0; - CacheOperationContext opCtx = cctx.operationContextPerCall(); - - // Force keepBinary for operation context to avoid binary deserialization inside entry processor - if (cctx.binaryMarshaller()) { - CacheOperationContext newOpCtx = null; - - if (opCtx == null) - // Mimics behavior of GridCacheAdapter#keepBinary and GridCacheProxyImpl#keepBinary - newOpCtx = new CacheOperationContext(false, null, true, null, false, null); - else if (!opCtx.isKeepBinary()) - newOpCtx = opCtx.keepBinary(); - - if (newOpCtx != null) - cctx.operationContextPerCall(newOpCtx); - } - Map> rows = new LinkedHashMap<>(); // Keys that failed to UPDATE due to concurrent updates. @@ -487,97 +471,93 @@ else if (!opCtx.isKeepBinary()) SQLException resEx = null; - try { - Iterator> it = cursor.iterator(); - - while (it.hasNext()) { - List e = it.next(); - Object key = e.get(0); - Object val = (hasNewVal ? e.get(valColIdx) : e.get(1)); + Iterator> it = cursor.iterator(); - Object newVal; + while (it.hasNext()) { + List e = it.next(); + Object key = e.get(0); + Object val = (hasNewVal ? e.get(valColIdx) : e.get(1)); - Map newColVals = new HashMap<>(); + Object newVal; - for (int i = 0; i < plan.colNames.length; i++) { - if (hasNewVal && i == valColIdx - 2) - continue; + Map newColVals = new HashMap<>(); - newColVals.put(plan.colNames[i], e.get(i + 2)); - } + for (int i = 0; i < plan.colNames.length; i++) { + if (hasNewVal && i == valColIdx - 2) + continue; - newVal = plan.valSupplier.apply(e); + newColVals.put(plan.colNames[i], e.get(i + 2)); + } - if (bin && !(val instanceof BinaryObject)) - val = cctx.grid().binary().toBinary(val); + newVal = plan.valSupplier.apply(e); - // Skip key and value - that's why we start off with 2nd column - for (int i = 0; i < plan.tbl.getColumns().length - 2; i++) { - Column c = plan.tbl.getColumn(i + 2); + if (bin && !(val instanceof BinaryObject)) + val = cctx.grid().binary().toBinary(val); - boolean hasNewColVal = newColVals.containsKey(c.getName()); + // Skip key and value - that's why we start off with 2nd column + for (int i = 0; i < plan.tbl.getColumns().length - 2; i++) { + Column c = plan.tbl.getColumn(i + 2); - // Binary objects get old field values from the Builder, so we can skip what we're not updating - if (bin && !hasNewColVal) - continue; + boolean hasNewColVal = newColVals.containsKey(c.getName()); - Object colVal = hasNewColVal ? newColVals.get(c.getName()) : desc.columnValue(key, val, i); + // Binary objects get old field values from the Builder, so we can skip what we're not updating + if (bin && !hasNewColVal) + continue; - desc.setColumnValue(key, newVal, colVal, i); - } + Object colVal = hasNewColVal ? newColVals.get(c.getName()) : desc.columnValue(key, val, i); - if (bin && hasProps) { - assert newVal instanceof BinaryObjectBuilder; + desc.setColumnValue(key, newVal, colVal, i); + } - newVal = ((BinaryObjectBuilder) newVal).build(); - } + if (bin && hasProps) { + assert newVal instanceof BinaryObjectBuilder; - Object srcVal = e.get(1); + newVal = ((BinaryObjectBuilder) newVal).build(); + } - if (bin && !(srcVal instanceof BinaryObject)) - srcVal = cctx.grid().binary().toBinary(srcVal); + Object srcVal = e.get(1); - rows.put(key, new ModifyingEntryProcessor(srcVal, new EntryValueUpdater(newVal))); + if (bin && !(srcVal instanceof BinaryObject)) + srcVal = cctx.grid().binary().toBinary(srcVal); - if ((pageSize > 0 && rows.size() == pageSize) || (!it.hasNext())) { - PageProcessingResult pageRes = processPage(cctx, rows); + rows.put(key, new ModifyingEntryProcessor(srcVal, new EntryValueUpdater(newVal))); - res += pageRes.cnt; + if ((pageSize > 0 && rows.size() == pageSize) || (!it.hasNext())) { + PageProcessingResult pageRes = processPage(cctx, rows); - failedKeys.addAll(F.asList(pageRes.errKeys)); + res += pageRes.cnt; - if (pageRes.ex != null) { - if (resEx == null) - resEx = pageRes.ex; - else - resEx.setNextException(pageRes.ex); - } + failedKeys.addAll(F.asList(pageRes.errKeys)); - if (it.hasNext()) - rows.clear(); // No need to clear after the last batch. + if (pageRes.ex != null) { + if (resEx == null) + resEx = pageRes.ex; + else + resEx.setNextException(pageRes.ex); } - } - if (resEx != null) { - if (!F.isEmpty(failedKeys)) { - // Don't go for a re-run if processing of some keys yielded exceptions and report keys that - // had been modified concurrently right away. - String msg = "Failed to UPDATE some keys because they had been modified concurrently " + - "[keys=" + failedKeys + ']'; + if (it.hasNext()) + rows.clear(); // No need to clear after the last batch. + } + } - SQLException dupEx = createJdbcSqlException(msg, IgniteQueryErrorCode.CONCURRENT_UPDATE); + if (resEx != null) { + if (!F.isEmpty(failedKeys)) { + // Don't go for a re-run if processing of some keys yielded exceptions and report keys that + // had been modified concurrently right away. + String msg = "Failed to UPDATE some keys because they had been modified concurrently " + + "[keys=" + failedKeys + ']'; - dupEx.setNextException(resEx); + SQLException dupEx = createJdbcSqlException(msg, IgniteQueryErrorCode.CONCURRENT_UPDATE); - resEx = dupEx; - } + dupEx.setNextException(resEx); - throw new IgniteSQLException(resEx); + resEx = dupEx; } + + throw new IgniteSQLException(resEx); } - finally { - cctx.operationContextPerCall(opCtx); - } + return new UpdateResult(res, failedKeys.toArray()); } diff --git a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java index 15c94c3d53f0d..549b901142589 100644 --- a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java +++ b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java @@ -99,7 +99,7 @@ private static UpdatePlan planForInsert(GridSqlStatement stmt) throws IgniteChec GridSqlColumn[] cols; - boolean isTableSubqry; + boolean isTwoStepSubqry; int rowsNum; @@ -116,8 +116,8 @@ private static UpdatePlan planForInsert(GridSqlStatement stmt) throws IgniteChec cols = ins.columns(); sel = DmlAstUtils.selectForInsertOrMerge(cols, ins.rows(), ins.query(), desc); - isTableSubqry = (ins.query() != null); - rowsNum = isTableSubqry ? 0 : ins.rows().size(); + isTwoStepSubqry = (ins.query() != null); + rowsNum = isTwoStepSubqry ? 0 : ins.rows().size(); } else if (stmt instanceof GridSqlMerge) { GridSqlMerge merge = (GridSqlMerge) stmt; @@ -137,14 +137,14 @@ else if (stmt instanceof GridSqlMerge) { cols = merge.columns(); sel = DmlAstUtils.selectForInsertOrMerge(cols, merge.rows(), merge.query(), desc); - isTableSubqry = (merge.query() != null); - rowsNum = isTableSubqry ? 0 : merge.rows().size(); + isTwoStepSubqry = (merge.query() != null); + rowsNum = isTwoStepSubqry ? 0 : merge.rows().size(); } else throw new IgniteSQLException("Unexpected DML operation [cls=" + stmt.getClass().getName() + ']', IgniteQueryErrorCode.UNEXPECTED_OPERATION); // Let's set the flag only for subqueries that have their FROM specified. - isTableSubqry = (isTableSubqry && (sel instanceof GridSqlUnion || + isTwoStepSubqry = (isTwoStepSubqry && (sel instanceof GridSqlUnion || (sel instanceof GridSqlSelect && ((GridSqlSelect) sel).from() != null))); int keyColIdx = -1; @@ -189,10 +189,10 @@ else throw new IgniteSQLException("Unexpected DML operation [cls=" + stmt.getCla if (stmt instanceof GridSqlMerge) return UpdatePlan.forMerge(tbl.dataTable(), colNames, keySupplier, valSupplier, keyColIdx, valColIdx, - sel.getSQL(), !isTableSubqry, rowsNum); + sel.getSQL(), !isTwoStepSubqry, rowsNum); else return UpdatePlan.forInsert(tbl.dataTable(), colNames, keySupplier, valSupplier, keyColIdx, valColIdx, - sel.getSQL(), !isTableSubqry, rowsNum); + sel.getSQL(), !isTwoStepSubqry, rowsNum); } /** From d8bb60b3a0f55b63e6406af30291bc713ba804d5 Mon Sep 17 00:00:00 2001 From: Alexander Paschenko Date: Sun, 27 Nov 2016 22:31:14 +0300 Subject: [PATCH 2/3] Test fix --- ...gniteCacheAbstractSqlDmlQuerySelfTest.java | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheAbstractSqlDmlQuerySelfTest.java b/modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheAbstractSqlDmlQuerySelfTest.java index 123c32a6a949a..28bc69a64df67 100644 --- a/modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheAbstractSqlDmlQuerySelfTest.java +++ b/modules/indexing/src/test/java/org/apache/ignite/internal/processors/cache/IgniteCacheAbstractSqlDmlQuerySelfTest.java @@ -89,25 +89,39 @@ private boolean isBinaryMarshaller() { startGridsMultiThreaded(3, true); ignite(0).createCache(cacheConfig("S2P", true, false).setIndexedTypes(String.class, Person.class)); - ignite(0).createCache(createBinCacheConfig()); + + if (isBinaryMarshaller()) + ignite(0).createCache(createBinCacheConfig()); } /** {@inheritDoc} */ @Override protected void beforeTest() throws Exception { super.beforeTest(); + + // Put to S2P only non binary Persons ignite(0).cache("S2P").put("FirstKey", new Person(1, "John", "White")); ignite(0).cache("S2P").put("SecondKey", new Person(2, "Joe", "Black")); ignite(0).cache("S2P").put("k3", new Person(3, "Sylvia", "Green")); ignite(0).cache("S2P").put("f0u4thk3y", new Person(4, "Jane", "Silver")); - ignite(0).cache("S2P-bin").put("FirstKey", createPerson(1, "John", "White")); - ignite(0).cache("S2P-bin").put("SecondKey", createPerson(2, "Joe", "Black")); - ignite(0).cache("S2P-bin").put("k3", createPerson(3, "Sylvia", "Green")); - ignite(0).cache("S2P-bin").put("f0u4thk3y", createPerson(4, "Jane", "Silver")); + if (isBinaryMarshaller()) { + ignite(0).cache("S2P-bin").put("FirstKey", createBinPerson(1, "John", "White")); + ignite(0).cache("S2P-bin").put("SecondKey", createBinPerson(2, "Joe", "Black")); + ignite(0).cache("S2P-bin").put("k3", createBinPerson(3, "Sylvia", "Green")); + ignite(0).cache("S2P-bin").put("f0u4thk3y", createBinPerson(4, "Jane", "Silver")); + } } /** */ Object createPerson(int id, String name, String secondName) { + if (!isBinaryMarshaller()) + return new Person(id, name, secondName); + else + return createBinPerson(id, name, secondName); + } + + /** */ + private Object createBinPerson(int id, String name, String secondName) { BinaryObjectBuilder bldr = ignite(0).binary().builder("Person"); bldr.setField("id", id); From 44d011b1a421d6d8f3d39e87f4f31ee7b06a3106 Mon Sep 17 00:00:00 2001 From: Alexander Paschenko Date: Mon, 28 Nov 2016 10:54:26 +0300 Subject: [PATCH 3/3] IGNITE-4312 Explicitly copy field values to builders in INSERT/MERGE and UPDATE. --- .../query/h2/DmlStatementsProcessor.java | 40 ++++++++++++++++++- .../query/h2/dml/UpdatePlanBuilder.java | 31 +++++++++++--- .../query/h2/opt/GridH2RowDescriptor.java | 2 +- 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java index 67e1f430235f7..45c830242a820 100644 --- a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java +++ b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/DmlStatementsProcessor.java @@ -53,6 +53,7 @@ import org.apache.ignite.internal.processors.query.GridQueryFieldMetadata; import org.apache.ignite.internal.processors.query.GridQueryFieldsResult; import org.apache.ignite.internal.processors.query.GridQueryFieldsResultAdapter; +import org.apache.ignite.internal.processors.query.GridQueryProperty; import org.apache.ignite.internal.processors.query.GridQueryTypeDescriptor; import org.apache.ignite.internal.processors.query.IgniteSQLException; import org.apache.ignite.internal.processors.query.h2.dml.FastUpdateArguments; @@ -476,6 +477,9 @@ private UpdateResult doUpdate(UpdatePlan plan, QueryCursorImpl> cursor, while (it.hasNext()) { List e = it.next(); Object key = e.get(0); + + // Base object to take "initial" column values from - either one currently in cache, + // or the one explicitly passed in arguments. Object val = (hasNewVal ? e.get(valColIdx) : e.get(1)); Object newVal; @@ -498,15 +502,25 @@ private UpdateResult doUpdate(UpdatePlan plan, QueryCursorImpl> cursor, for (int i = 0; i < plan.tbl.getColumns().length - 2; i++) { Column c = plan.tbl.getColumn(i + 2); + GridQueryProperty prop = desc.type().property(c.getName()); + + if (prop.key()) + continue; // Don't get values of key's columns - we won't use them anyway + boolean hasNewColVal = newColVals.containsKey(c.getName()); + // TODO uncomment when https://issues.apache.org/jira/browse/IGNITE-4312 is fixed + /* // Binary objects get old field values from the Builder, so we can skip what we're not updating if (bin && !hasNewColVal) continue; + */ - Object colVal = hasNewColVal ? newColVals.get(c.getName()) : desc.columnValue(key, val, i); + // Column values that have been explicitly specified have priority over field values in old or new _val + Object colVal = hasNewColVal ? newColVals.get(c.getName()) : desc.columnValue(null, val, i); - desc.setColumnValue(key, newVal, colVal, i); + // UPDATE currently does not allow to modify key or its fields, so we must be safe to pass null as key + desc.setColumnValue(null, newVal, colVal, i); } if (bin && hasProps) { @@ -785,6 +799,28 @@ private static PageProcessingResult processPage(GridCacheContext cctx, if (val == null) throw new IgniteSQLException("Value for INSERT or MERGE must not be null", IgniteQueryErrorCode.NULL_VALUE); + // TODO remove this whole condition when https://issues.apache.org/jira/browse/IGNITE-4312 is fixed + boolean isKeyBldr = key instanceof BinaryObjectBuilder; + boolean isValBldr = val instanceof BinaryObjectBuilder; + if (cctx.binaryMarshaller() && (isKeyBldr || isValBldr)) { + Object explicitKey = keyColIdx != -1 ? row[keyColIdx] : null; + + Object explicitVal = valColIdx != -1 ? row[valColIdx] : null; + + Set propNames = desc.fields().keySet(); + + for (String propName : propNames) { + GridQueryProperty prop = desc.property(propName); + + assert prop != null; + + if (prop.key() && explicitKey != null && isKeyBldr) + prop.setValue(key, null, prop.value(explicitKey, null)); + else if (!prop.key() && explicitVal != null && isValBldr) + prop.setValue(null, val, prop.value(null, explicitVal)); + } + } + for (int i = 0; i < cols.length; i++) { if (i == keyColIdx || i == valColIdx) continue; diff --git a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java index 549b901142589..cb77e35ab4c36 100644 --- a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java +++ b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/dml/UpdatePlanBuilder.java @@ -23,7 +23,6 @@ import java.util.Set; import javax.cache.CacheException; import org.apache.ignite.IgniteCheckedException; -import org.apache.ignite.binary.BinaryObject; import org.apache.ignite.internal.processors.cache.GridCacheContext; import org.apache.ignite.internal.processors.cache.query.IgniteQueryErrorCode; import org.apache.ignite.internal.processors.query.GridQueryProcessor; @@ -245,8 +244,6 @@ else if (stmt instanceof GridSqlDelete) { GridSqlSelect sel; if (stmt instanceof GridSqlUpdate) { - boolean bin = desc.context().binaryMarshaller(); - List updatedCols = ((GridSqlUpdate) stmt).cols(); int valColIdx = -1; @@ -270,6 +267,15 @@ else if (stmt instanceof GridSqlDelete) { if (hasNewVal) valColIdx += 2; + // TODO https://issues.apache.org/jira/browse/IGNITE-4312 + /* + + This section has been commented out due to the issue linked above - + without this problem, in case of binary marshaller we could use either explicitly use either old _val + or explicitly set new _val as base object whose field values we would update, but because of it, + we have to instantiate new binary objects explicitly and just copy field values one by one. + Sad but true. When the issue is fixed, this section will have to be uncommented. + int newValColIdx; if (!hasProps) // No distinct properties, only whole new value - let's take it @@ -284,6 +290,9 @@ else if (bin) // We update distinct columns in binary mode - let's choose correc // Otherwise we always want it to instantiate new object whose properties we will later // set to current values. KeyValueSupplier newValSupplier = createSupplier(desc.context(), desc.type(), newValColIdx, hasProps, false); + */ + + KeyValueSupplier newValSupplier = createSupplier(desc.context(), desc.type(), -1, hasProps, false); sel = DmlAstUtils.selectForUpdate((GridSqlUpdate) stmt, errKeysPos); @@ -335,10 +344,17 @@ else if (isSqlType) } if (cctx.binaryMarshaller()) { + // TODO https://issues.apache.org/jira/browse/IGNITE-4312 + /* + This section has been commented out due to the issue linked above - commented code is correct, and + without this problem, in case of binary marshaller we could use either explicitly use either old _val + or explicitly set new _val as base object whose field values we would update, but because of it, + we have to instantiate new binary objects explicitly and just copy field values one by one. + Sad but true. When the issue is fixed, this section will have to be uncommented. + if (colIdx != -1) { // If we have key or value explicitly present in query, create new builder upon them... return new KeyValueSupplier() { - /** {@inheritDoc} */ @Override public Object apply(List arg) throws IgniteCheckedException { BinaryObject bin = cctx.grid().binary().toBinary(arg.get(colIdx)); @@ -349,12 +365,17 @@ else if (isSqlType) else { // ...and if we don't, just create a new builder. return new KeyValueSupplier() { - /** {@inheritDoc} */ @Override public Object apply(List arg) throws IgniteCheckedException { return cctx.grid().binary().builder(typeName); } }; } + */ + return new KeyValueSupplier() { + @Override public Object apply(List arg) throws IgniteCheckedException { + return cctx.grid().binary().builder(typeName); + } + }; } else { Constructor ctor; diff --git a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2RowDescriptor.java b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2RowDescriptor.java index 3465ed7cc8c92..0161ff71ce4cc 100644 --- a/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2RowDescriptor.java +++ b/modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/opt/GridH2RowDescriptor.java @@ -117,7 +117,7 @@ public GridH2Row createRow(CacheObject key, @Nullable CacheObject val, long expi public void setColumnValue(Object key, Object val, Object colVal, int col); /** - * Determine whether a column corresponds to a property of key or to one of value. + * Determine whether a column corresponds to a property of key or of value. * * @param col Column index. * @return {@code true} if given column corresponds to a key property, {@code false} otherwise