You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2023/10/10 16:06:12 UTC
[PR] IGNITE-20454 [ignite-3]
xtern opened a new pull request, #2674:
URL: https://github.com/apache/ignite-3/pull/2674
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356692257
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
+
+ // Make sure all data is fetched successfully.
+ assertThat(fetchAll(prepareTestCursor(listener)), is(true));
+
+ assertThat(listener.callsCountAsync(), willBe(0));
+
+ latch.countDown();
+
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ private static AsyncCursor<Integer> prepareTestCursor(PageLoadListener listener) {
+ AsyncCursor<Integer> data = new AsyncWrapper<>(IntStream.range(0, 10).boxed().collect(Collectors.toList()).iterator());
+ QueryTransactionWrapper tx = new QueryTransactionWrapper(NoOpTransaction.readOnly("TX"), true);
+ ResultSetMetadata rsMeta = new ResultSetMetadataImpl(Collections.emptyList());
+
+ return new AsyncSqlScriptCursor<>(SqlQueryType.QUERY, rsMeta, tx, data, listener);
+ }
+
+ private static boolean fetchAll(AsyncCursor<Integer> cursor) throws ExecutionException, InterruptedException, TimeoutException {
Review Comment:
Done
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356612831
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlCursorImplTest.java:
##########
@@ -45,27 +58,26 @@
* Tests for {@link AsyncSqlCursorImpl}.
*/
public class AsyncSqlCursorImplTest {
-
private static final ResultSetMetadata RESULT_SET_METADATA = new ResultSetMetadataImpl(Collections.emptyList());
+ private static final List<Integer> ROWS = IntStream.range(0, 10).boxed().collect(Collectors.toList());
/** Cursor should trigger commit of implicit transaction (if any) only if data is fully read. */
@ParameterizedTest(name = "{0}")
@MethodSource("transactions")
public void testTriggerCommitAfterDataIsFullyRead(boolean implicit, QueryTransactionWrapper txWrapper) {
- List<Integer> list = List.of(1, 2, 3);
AsyncSqlCursorImpl<Integer> cursor = new AsyncSqlCursorImpl<>(SqlQueryType.QUERY, RESULT_SET_METADATA, txWrapper,
- new AsyncWrapper<>(CompletableFuture.completedFuture(list.iterator()), Runnable::run));
+ new AsyncWrapper<>(CompletableFuture.completedFuture(ROWS.iterator()), Runnable::run));
int requestRows = 2;
BatchedResult<Integer> in1 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in1.items(), list.subList(0, requestRows));
+ assertEquals(ROWS.subList(0, requestRows), in1.items());
Review Comment:
swapped args to (expected, result)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlCursorImplTest.java:
##########
@@ -45,27 +58,26 @@
* Tests for {@link AsyncSqlCursorImpl}.
*/
public class AsyncSqlCursorImplTest {
-
private static final ResultSetMetadata RESULT_SET_METADATA = new ResultSetMetadataImpl(Collections.emptyList());
+ private static final List<Integer> ROWS = IntStream.range(0, 10).boxed().collect(Collectors.toList());
/** Cursor should trigger commit of implicit transaction (if any) only if data is fully read. */
@ParameterizedTest(name = "{0}")
@MethodSource("transactions")
public void testTriggerCommitAfterDataIsFullyRead(boolean implicit, QueryTransactionWrapper txWrapper) {
- List<Integer> list = List.of(1, 2, 3);
AsyncSqlCursorImpl<Integer> cursor = new AsyncSqlCursorImpl<>(SqlQueryType.QUERY, RESULT_SET_METADATA, txWrapper,
- new AsyncWrapper<>(CompletableFuture.completedFuture(list.iterator()), Runnable::run));
+ new AsyncWrapper<>(CompletableFuture.completedFuture(ROWS.iterator()), Runnable::run));
int requestRows = 2;
BatchedResult<Integer> in1 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in1.items(), list.subList(0, requestRows));
+ assertEquals(ROWS.subList(0, requestRows), in1.items());
assertFalse(((NoOpTransaction) txWrapper.unwrap()).commitFuture().isDone(),
"Implicit transaction should have not been committed because there is more data.");
- BatchedResult<Integer> in2 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in2.items(), list.subList(requestRows, list.size()));
+ BatchedResult<Integer> in2 = cursor.requestNextAsync(ROWS.size()).join();
+ assertEquals(ROWS.subList(requestRows, ROWS.size()), in2.items());
Review Comment:
swapped args to (expected, result)
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356705589
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
Review Comment:
Done
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1355022300
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
Review Comment:
I think that timed variant of await is unnecessary here and it can be replaced with latch::await.
--
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
Re: [PR] IGNITE-20454 Sql. Add a query prefetch callback that is notified when data prefetching is complete. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern merged PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#issuecomment-1757723219
@xtern Maybe we can add a test case for `ExecutionServiceImplTest` that exercises that this listener is called?
--
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
Re: [PR] IGNITE-20454 Sql. Add a query prefetch callback that is notified when data prefetching is complete. [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#issuecomment-1766399895
> The goal of the JIRA is to eliminate dependency on the user's input while processing the script. `AsyncSqlCursorImpl#requestNextAsync` is a user input for sure. Besides, the description of the ticket points out to a particular place that triggers prefetch (`AsyncRootNode#prefetch`). End of the prefetch is the moment when `waiting` reaches 0 for the first time. So, we need to trigger listeners when `waiting` reaches 0 after `AsyncRootNode#prefetch` was invoked.
>
> BTW, I think, the name `listener` is more suitable for recurrent events, while for single shot notification 'callback' fits better
@korlov42
Yes :woman_facepalming:, thanks for the detailed explanation.
I've reworked the patch, please have a look.
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356586797
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
+
+ // Make sure all data is fetched successfully.
+ assertThat(fetchAll(prepareTestCursor(listener)), is(true));
+
+ assertThat(listener.callsCountAsync(), willBe(0));
+
+ latch.countDown();
+
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ private static AsyncCursor<Integer> prepareTestCursor(PageLoadListener listener) {
+ AsyncCursor<Integer> data = new AsyncWrapper<>(IntStream.range(0, 10).boxed().collect(Collectors.toList()).iterator());
+ QueryTransactionWrapper tx = new QueryTransactionWrapper(NoOpTransaction.readOnly("TX"), true);
+ ResultSetMetadata rsMeta = new ResultSetMetadataImpl(Collections.emptyList());
+
+ return new AsyncSqlScriptCursor<>(SqlQueryType.QUERY, rsMeta, tx, data, listener);
+ }
+
+ private static boolean fetchAll(AsyncCursor<Integer> cursor) throws ExecutionException, InterruptedException, TimeoutException {
Review Comment:
Sorry, not done yet)
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "lowka (via GitHub)" <gi...@apache.org>.
lowka commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1355021110
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
+
+ // Make sure all data is fetched successfully.
+ assertThat(fetchAll(prepareTestCursor(listener)), is(true));
+
+ assertThat(listener.callsCountAsync(), willBe(0));
+
+ latch.countDown();
+
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ private static AsyncCursor<Integer> prepareTestCursor(PageLoadListener listener) {
+ AsyncCursor<Integer> data = new AsyncWrapper<>(IntStream.range(0, 10).boxed().collect(Collectors.toList()).iterator());
+ QueryTransactionWrapper tx = new QueryTransactionWrapper(NoOpTransaction.readOnly("TX"), true);
+ ResultSetMetadata rsMeta = new ResultSetMetadataImpl(Collections.emptyList());
+
+ return new AsyncSqlScriptCursor<>(SqlQueryType.QUERY, rsMeta, tx, data, listener);
+ }
+
+ private static boolean fetchAll(AsyncCursor<Integer> cursor) throws ExecutionException, InterruptedException, TimeoutException {
Review Comment:
I think we do not need the result of this method + this method can be rewritten into `do { .. } while(res.hasMore())`.
It also does not need timed variant of Future::get.
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356574574
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlScriptCursorTest.java:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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;
+
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willBe;
+import static org.apache.ignite.internal.testframework.matchers.CompletableFutureMatcher.willCompleteSuccessfully;
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
+import java.util.concurrent.CompletionException;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+import java.util.stream.IntStream;
+import org.apache.ignite.internal.sql.api.ResultSetMetadataImpl;
+import org.apache.ignite.internal.sql.engine.framework.NoOpTransaction;
+import org.apache.ignite.internal.testframework.IgniteTestUtils.RunnableX;
+import org.apache.ignite.internal.util.AsyncCursor;
+import org.apache.ignite.internal.util.AsyncCursor.BatchedResult;
+import org.apache.ignite.internal.util.AsyncWrapper;
+import org.apache.ignite.sql.ResultSetMetadata;
+import org.junit.jupiter.api.Test;
+
+/**
+ * Tests for {@link AsyncSqlScriptCursor}.
+ */
+public class AsyncSqlScriptCursorTest {
+ private static final int PAGE_SIZE = 1;
+
+ @Test
+ public void listenerIsCalledOnFirstPageOnly() throws Exception {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {});
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ assertThat(listener.callsCounter.get(), is(0));
+
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ assertThat(listener.callsCountAsync(), willBe(1));
+
+ assertThat(fetchAll(cursor), is(true));
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ @Test
+ public void testErrorInPrefetchListenerDoesNotAffectExecution() {
+ TestPageLoadListener listener = new TestPageLoadListener(() -> {
+ throw new RuntimeException("Expected");
+ });
+
+ AsyncCursor<Integer> cursor = prepareTestCursor(listener);
+
+ // Ensure request future completes without errors.
+ assertThat(cursor.requestNextAsync(PAGE_SIZE), willCompleteSuccessfully());
+ }
+
+ @Test
+ public void testPrefetchListenerExecutesAsynchronously() throws Exception {
+ CountDownLatch latch = new CountDownLatch(1);
+ TestPageLoadListener listener = new TestPageLoadListener(() -> latch.await(30, TimeUnit.SECONDS));
+
+ // Make sure all data is fetched successfully.
+ assertThat(fetchAll(prepareTestCursor(listener)), is(true));
+
+ assertThat(listener.callsCountAsync(), willBe(0));
+
+ latch.countDown();
+
+ assertThat(listener.callsCountAsync(), willBe(1));
+ }
+
+ private static AsyncCursor<Integer> prepareTestCursor(PageLoadListener listener) {
+ AsyncCursor<Integer> data = new AsyncWrapper<>(IntStream.range(0, 10).boxed().collect(Collectors.toList()).iterator());
+ QueryTransactionWrapper tx = new QueryTransactionWrapper(NoOpTransaction.readOnly("TX"), true);
+ ResultSetMetadata rsMeta = new ResultSetMetadataImpl(Collections.emptyList());
+
+ return new AsyncSqlScriptCursor<>(SqlQueryType.QUERY, rsMeta, tx, data, listener);
+ }
+
+ private static boolean fetchAll(AsyncCursor<Integer> cursor) throws ExecutionException, InterruptedException, TimeoutException {
Review Comment:
Done
--
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
Re: [PR] IGNITE-20454 Sql. Extend SQL cursor with ability to check if first page is ready [ignite-3]
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2674:
URL: https://github.com/apache/ignite-3/pull/2674#discussion_r1356613011
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlCursorImplTest.java:
##########
@@ -45,27 +58,26 @@
* Tests for {@link AsyncSqlCursorImpl}.
*/
public class AsyncSqlCursorImplTest {
-
private static final ResultSetMetadata RESULT_SET_METADATA = new ResultSetMetadataImpl(Collections.emptyList());
+ private static final List<Integer> ROWS = IntStream.range(0, 10).boxed().collect(Collectors.toList());
/** Cursor should trigger commit of implicit transaction (if any) only if data is fully read. */
@ParameterizedTest(name = "{0}")
@MethodSource("transactions")
public void testTriggerCommitAfterDataIsFullyRead(boolean implicit, QueryTransactionWrapper txWrapper) {
- List<Integer> list = List.of(1, 2, 3);
AsyncSqlCursorImpl<Integer> cursor = new AsyncSqlCursorImpl<>(SqlQueryType.QUERY, RESULT_SET_METADATA, txWrapper,
- new AsyncWrapper<>(CompletableFuture.completedFuture(list.iterator()), Runnable::run));
+ new AsyncWrapper<>(CompletableFuture.completedFuture(ROWS.iterator()), Runnable::run));
int requestRows = 2;
BatchedResult<Integer> in1 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in1.items(), list.subList(0, requestRows));
+ assertEquals(ROWS.subList(0, requestRows), in1.items());
assertFalse(((NoOpTransaction) txWrapper.unwrap()).commitFuture().isDone(),
"Implicit transaction should have not been committed because there is more data.");
- BatchedResult<Integer> in2 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in2.items(), list.subList(requestRows, list.size()));
+ BatchedResult<Integer> in2 = cursor.requestNextAsync(ROWS.size()).join();
+ assertEquals(ROWS.subList(requestRows, ROWS.size()), in2.items());
Review Comment:
swapped args from (result, expected) to (expected, result)
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/AsyncSqlCursorImplTest.java:
##########
@@ -45,27 +58,26 @@
* Tests for {@link AsyncSqlCursorImpl}.
*/
public class AsyncSqlCursorImplTest {
-
private static final ResultSetMetadata RESULT_SET_METADATA = new ResultSetMetadataImpl(Collections.emptyList());
+ private static final List<Integer> ROWS = IntStream.range(0, 10).boxed().collect(Collectors.toList());
/** Cursor should trigger commit of implicit transaction (if any) only if data is fully read. */
@ParameterizedTest(name = "{0}")
@MethodSource("transactions")
public void testTriggerCommitAfterDataIsFullyRead(boolean implicit, QueryTransactionWrapper txWrapper) {
- List<Integer> list = List.of(1, 2, 3);
AsyncSqlCursorImpl<Integer> cursor = new AsyncSqlCursorImpl<>(SqlQueryType.QUERY, RESULT_SET_METADATA, txWrapper,
- new AsyncWrapper<>(CompletableFuture.completedFuture(list.iterator()), Runnable::run));
+ new AsyncWrapper<>(CompletableFuture.completedFuture(ROWS.iterator()), Runnable::run));
int requestRows = 2;
BatchedResult<Integer> in1 = cursor.requestNextAsync(requestRows).join();
- assertEquals(in1.items(), list.subList(0, requestRows));
+ assertEquals(ROWS.subList(0, requestRows), in1.items());
Review Comment:
swapped args from (result, expected) to (expected, result)
--
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