Copilot commented on code in PR #7949:
URL: https://github.com/apache/ignite-3/pull/7949#discussion_r3049877003
##########
modules/client-handler/src/main/java/org/apache/ignite/client/handler/requests/sql/ClientSqlCursorNextResultRequest.java:
##########
@@ -38,42 +39,49 @@ public class ClientSqlCursorNextResultRequest {
* Processes the request.
*
* @param in Unpacker.
+ * @param requestTsTracker TS tracker attached to request processing
* @return Future representing result of operation.
*/
public static CompletableFuture<ResponseWriter> process(
- ClientMessageUnpacker in,
+ Executor operationExecutor, ClientMessageUnpacker in,
ClientResourceRegistry resources,
- Executor operationExecutor,
- ClientHandlerMetricSource metrics
+ ClientHandlerMetricSource metrics,
+ HybridTimestampTracker requestTsTracker
Review Comment:
Javadoc is out of sync with the method signature: `process` now takes
`operationExecutor` as the first argument, but it is not documented in the
`@param` list, and the existing `@param in` description no longer matches the
signature formatting. Please update the Javadoc to document `operationExecutor`
and keep parameter docs aligned with the actual signature.
##########
modules/jdbc/src/testFixtures/java/org/apache/ignite/jdbc/util/RowsProjectionMatcher.java:
##########
@@ -0,0 +1,273 @@
+/*
+ * 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.jdbc.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.hamcrest.Description;
+import org.hamcrest.SelfDescribing;
+import org.hamcrest.TypeSafeMatcher;
+import org.jetbrains.annotations.Nullable;
+
+/** Matcher for result set rows column projection. */
+public abstract class RowsProjectionMatcher<T> extends
TypeSafeMatcher<List<T>> {
+
+ private @Nullable MismatchDescriber mismatch;
+
+ @Override
+ protected final boolean matchesSafely(List<T> projection) {
+ try {
+ doMatch(projection);
+ return true;
+
+ } catch (MismatchException e) {
+ mismatch = e.describer();
+ return false;
+ }
+ }
+
+ /**
+ * Performs the actual matching.
+ *
+ * @throws MismatchException on first mismatch.
+ */
+ abstract void doMatch(List<T> projection) throws MismatchException;
+
+ @Override
+ protected void describeMismatchSafely(List<T> projection, Description
description) {
+ assert mismatch != null;
+ description.appendText("Actual projection " + projection + " ");
+ mismatch.describeTo(description);
+ }
+
+ /** Matcher verifying that checked projection contains all values from
provided list. */
+ @SafeVarargs
+ public static <T> RowsProjectionMatcher<T> hasValuesInAnyOrder(T...
expectedVals) {
+ return hasValues("Rows projection with following values in any order
", (projection, expectedProjection) -> {
+
+ Map<T, Integer> remaining = new HashMap<>();
+ for (T value : expectedProjection) {
+ remaining.merge(value, 1, Integer::sum);
+ }
+
+ int expectedSize = expectedProjection.size();
+ int actualSize = projection.size();
+
+ for (int index = 0; index < actualSize; index++) {
+
+ if (index > expectedSize) {
+ throw MismatchException.tooManyRecords(actualSize,
expectedSize,
+ projection.subList(index, actualSize)
+ );
+ }
+
Review Comment:
Off-by-one in the size guard: `index > expectedSize` will not trigger when
`actualSize == expectedSize + 1` at `index == expectedSize`, so extra rows are
reported as `unexpectedValue` (or can lead to other errors) instead of
`tooManyRecords`. Use a 0-based check (`index >= expectedSize`) or check
`actualSize > expectedSize` before iterating, and then report the redundant
tail starting at `expectedSize`.
##########
modules/jdbc/src/testFixtures/java/org/apache/ignite/jdbc/util/RowsProjectionMatcher.java:
##########
@@ -0,0 +1,273 @@
+/*
+ * 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.jdbc.util;
+
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Collectors;
+import org.hamcrest.Description;
+import org.hamcrest.SelfDescribing;
+import org.hamcrest.TypeSafeMatcher;
+import org.jetbrains.annotations.Nullable;
+
+/** Matcher for result set rows column projection. */
+public abstract class RowsProjectionMatcher<T> extends
TypeSafeMatcher<List<T>> {
+
+ private @Nullable MismatchDescriber mismatch;
+
+ @Override
+ protected final boolean matchesSafely(List<T> projection) {
+ try {
+ doMatch(projection);
+ return true;
+
+ } catch (MismatchException e) {
+ mismatch = e.describer();
+ return false;
+ }
+ }
+
+ /**
+ * Performs the actual matching.
+ *
+ * @throws MismatchException on first mismatch.
+ */
+ abstract void doMatch(List<T> projection) throws MismatchException;
+
+ @Override
+ protected void describeMismatchSafely(List<T> projection, Description
description) {
+ assert mismatch != null;
+ description.appendText("Actual projection " + projection + " ");
+ mismatch.describeTo(description);
+ }
+
+ /** Matcher verifying that checked projection contains all values from
provided list. */
+ @SafeVarargs
+ public static <T> RowsProjectionMatcher<T> hasValuesInAnyOrder(T...
expectedVals) {
+ return hasValues("Rows projection with following values in any order
", (projection, expectedProjection) -> {
+
+ Map<T, Integer> remaining = new HashMap<>();
+ for (T value : expectedProjection) {
+ remaining.merge(value, 1, Integer::sum);
+ }
+
+ int expectedSize = expectedProjection.size();
+ int actualSize = projection.size();
+
+ for (int index = 0; index < actualSize; index++) {
+
+ if (index > expectedSize) {
+ throw MismatchException.tooManyRecords(actualSize,
expectedSize,
+ projection.subList(index, actualSize)
+ );
+ }
+
+ T actual = projection.get(index);
+ Integer count = remaining.get(actual);
+
+ if (count == null) {
+ throw MismatchException.unexpectedValue(actual, index,
remaining.entrySet().stream()
+ .flatMap(e -> Collections.nCopies(e.getValue(),
e.getKey()).stream())
+ .collect(Collectors.toList()));
+ }
+
+ if (count == 1) {
+ remaining.remove(actual);
+ } else {
+ remaining.put(actual, count - 1);
+ }
+ }
+
+ if (!remaining.isEmpty()) {
+ throw MismatchException.notEnoughRecords(actualSize,
expectedSize,
+ remaining.entrySet().stream()
+ .flatMap(e ->
Collections.nCopies(e.getValue(), e.getKey()).stream())
+ .collect(Collectors.toList()));
+ }
+
+ }, expectedVals);
+ }
+
+ /** Matcher verifying that checked projection satisfies provided values
order. */
+ @SafeVarargs
+ public static <T> RowsProjectionMatcher<T> hasValuesOrder(T...
expectedVals) {
+ return hasValues("Rows projection with following values order ",
(projection, expectedProjection) -> {
+
+ int expectedSize = expectedProjection.size();
+ int actualSize = projection.size();
+
+ int index;
+
+ for (index = 0; index < actualSize; index++) {
+
+ if (index > expectedSize) {
+ throw MismatchException.tooManyRecords(actualSize,
expectedSize,
+ projection.subList(index, actualSize)
+ );
+ }
+
+ T actual = projection.get(index);
+ T expected = expectedProjection.get(index);
+
+ if (!Objects.equals(actual, expected)) {
+ throw MismatchException.unexpectedValue(actual, index,
expected);
+ }
Review Comment:
Off-by-one in the bounds check: `index > expectedSize` should be `index >=
expectedSize` (0-based). With the current condition, when the actual projection
has more rows than expected, the code reaches `expectedProjection.get(index)`
at `index == expectedSize` and throws `IndexOutOfBoundsException` instead of
producing a clear `tooManyRecords` mismatch.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
