alex-plekhanov commented on code in PR #11613:
URL: https://github.com/apache/ignite/pull/11613#discussion_r1815509156
##########
modules/indexing/src/test/java/org/apache/ignite/client/ClientTestSuite.java:
##########
@@ -98,6 +99,7 @@
ClusterGroupClusterRestartTest.class,
BlockingTxOpsTest.class,
InvokeTest.class,
+ ExtraColumnInH2RowsTest.class
Review Comment:
Comma at the EOL
##########
modules/indexing/src/main/java/org/apache/ignite/internal/processors/query/h2/twostep/AbstractReducer.java:
##########
@@ -319,20 +320,23 @@ protected final ReduceResultPage
createDummyLastPage(ReduceResultPage lastPage)
* @param iter Current iterator.
* @return The same or new iterator.
*/
- protected final Iterator<Value[]>
pollNextIterator(Pollable<ReduceResultPage> queue, Iterator<Value[]> iter) {
- if (!iter.hasNext()) {
+ protected final IgniteIntObjectTuple<Iterator<Value[]>> pollNextIterator(
+ Pollable<ReduceResultPage> queue,
+ IgniteIntObjectTuple<Iterator<Value[]>> iter
+ ) {
+ if (!iter.get2().hasNext()) {
try (TraceSurroundings ignored =
MTC.support(ctx.tracing().create(SQL_PAGE_FETCH, MTC.span()))) {
ReduceResultPage page = takeNextPage(queue);
if (!page.isLast())
page.fetchNextPage(); // Failed will throw an exception
here.
- iter = page.rows();
+ iter = F.intt(page.columnCount(), page.rows());
Review Comment:
`page.columnCount()` is the same for all pages, looks like it's more correct
to store it in reducer. Also it will reduce GC pressure. WDYT?
##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/h2/twostep/messages/GridQueryNextPageResponse.java:
##########
@@ -60,14 +60,18 @@ public class GridQueryNextPageResponse implements Message {
@GridDirectCollection(Message.class)
private Collection<Message> vals;
- /** */
+ /**
+ * Note, columns count in plain row can differ from {@link #cols}.
+ * See {@code
org.apache.ignite.internal.processors.query.h2.twostep.msg.GridH2ValueMessageFactory#toMessages}.
Review Comment:
Can we also change `GridMapQueryExecutor.prepareNextPage` with this patch?
Now we have this call:
```
toMessages(rows, new ArrayList<>(res.columnCount()), res.columnCount())
```
But actially we should preallocate `res.columnCount() * rows.size()` items,
to avoid resizing.
##########
modules/core/src/main/java/org/apache/ignite/internal/util/lang/IgniteIntObjectTuple.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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.util.lang;
+
+import java.io.Externalizable;
+import java.util.Objects;
+import org.apache.ignite.internal.util.tostring.GridToStringInclude;
+import org.apache.ignite.internal.util.typedef.F;
+import org.apache.ignite.internal.util.typedef.internal.S;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Convenience class representing mutable tuple of two values when first is
int primitive.
+ */
+public class IgniteIntObjectTuple<V> {
Review Comment:
I think this is redundant class for current ticket. `val1` used as columns
count. Columns count almost never will be more than 127. Integer values up to
127 by default is cached by JVM (see `Integer.valueOf`). So, there is no
overhead for `IgniteBiTuple<Integer, V>` compared to `IgniteIntObjectTuple<V>`.
No GC pressure (since new Integer objects are not created), no memory footprint
change (since reference to `Integer` costs the same 4 bytes as `int`).
##########
modules/indexing/src/test/java/org/apache/ignite/client/ClientTestSuite.java:
##########
@@ -47,6 +47,7 @@
import
org.apache.ignite.internal.client.thin.events.IgniteClientConnectionEventListenerTest;
import
org.apache.ignite.internal.client.thin.events.IgniteClientLifecycleEventListenerTest;
import
org.apache.ignite.internal.client.thin.events.IgniteClientRequestEventListenerTest;
+import org.apache.ignite.sqltests.ExtraColumnInH2RowsTest;
Review Comment:
This test suite related to thin client tests. So, either test should be in
correspoinding thin client package, or should be moved to another suite
(related to SQL tests).
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
