You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/02/16 12:29:00 UTC
[GitHub] [ignite-3] korlov42 commented on a change in pull request #662: IGNITE-16547 MERGE operation support.
korlov42 commented on a change in pull request #662:
URL: https://github.com/apache/ignite-3/pull/662#discussion_r807834384
##########
File path: modules/schema/src/main/java/org/apache/ignite/internal/schema/row/ExpandableByteBuf.java
##########
@@ -172,12 +172,12 @@ public void putBytes(int off, byte[] val) {
public int putString(int off, String val, CharsetEncoder encoder) throws CharacterCodingException {
ensureCapacity(off);
- buf.position(off);
-
if (val.isEmpty()) {
return 0;
}
+ buf.position(off);
Review comment:
You just hide the problem. Please revert this change.
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/ModifyTuple.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.engine.schema;
+
+import org.apache.calcite.rel.core.TableModify;
+import org.apache.ignite.internal.schema.BinaryRow;
+
+/** POJO to store row and modify operation. */
+public class ModifyTuple {
+ private final BinaryRow row;
+
+ private final TableModify.Operation op;
Review comment:
Let's introduce a internal enum `Type` (or something like this) instead of `TableModify.Operation`
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java
##########
@@ -355,7 +380,11 @@ public void removeIndex(String idxName) {
assert !colDesc.physicalType().spec().fixedLength();
- int colIdInRow = columnToIndex.getOrDefault(colDesc.name(), colDesc.logicalIndex());
+ int colIdInRow = columnToIndex.getOrDefault(colDesc.name(), -1);
Review comment:
why don't just add the offset?
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/TableScanNode.java
##########
@@ -218,7 +224,7 @@ private void requestNextBatch() {
}
private class SubscriberImpl implements Flow.Subscriber<BinaryRow> {
- private int received;
+ int received = 0; // HB defenced here.
Review comment:
why do you remove the access modifier?
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/ModifyTuple.java
##########
@@ -0,0 +1,41 @@
+/*
+ * 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.sql.engine.schema;
+
+import org.apache.calcite.rel.core.TableModify;
+import org.apache.ignite.internal.schema.BinaryRow;
+
+/** POJO to store row and modify operation. */
+public class ModifyTuple {
Review comment:
does it make sense to rename it to `ModifyRow`?
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/InternalIgniteTable.java
##########
@@ -104,13 +104,13 @@ IgniteLogicalIndexScan toRel(
* @param row Relational node row.
* @param op Operation.
* @param arg Operation specific argument.
- * @return Cache key-value tuple;
+ * @return Cache key-value tuple with modify operation info;
*/
- <RowT> BinaryRow toBinaryRow(
+ <RowT> ModifyTuple toBinaryRow(
Review comment:
`toModifyTuple`
##########
File path: modules/schema/src/test/java/org/apache/ignite/internal/schema/RowTest.java
##########
@@ -697,6 +697,10 @@ private void checkValues(SchemaDescriptor schema, Object... vals) {
}
}
+ BinaryRow row0 = asm.build();
+
+ assert vals.length > 0 && row0.bytes().length > 0;
Review comment:
Let's write a particular test to validate `org.apache.ignite.internal.schema.ByteBufferRow#bytes`
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteResource.java
##########
@@ -32,6 +32,9 @@
@Resources.BaseMessage("Cannot update field \"{0}\". You cannot update key, key fields or val field in case the val is a complex type.")
Resources.ExInst<SqlValidatorException> cannotUpdateField(String field);
+ @Resources.BaseMessage("Cannot update field \"{0}\". Attempt to update primary keys partially.")
+ Resources.ExInst<SqlValidatorException> cannotUpdatePkPartially(String field);
Review comment:
Let's move all this partial-pk-update things to a separate PR
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/TableScanNode.java
##########
@@ -184,7 +184,13 @@ private void push() throws Exception {
}
}
+ assert waiting >= 0;
Review comment:
I don't sure it's a good idea to put assertions randomly in the code. If need to validate some condition, then put validation as close as it possible to the place where the invariant of interest might be put to an invalid state
##########
File path: modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/rel/TableScanNode.java
##########
@@ -184,7 +184,13 @@ private void push() throws Exception {
}
}
+ assert waiting >= 0;
+
if (waiting == 0 || activeSubscription == null) {
+ if (activeSubscription == null) {
+ waiting = 0;
Review comment:
why do you need to reset the counter here?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org