You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by bijugs <gi...@git.apache.org> on 2017/01/10 19:55:15 UTC
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
GitHub user bijugs opened a pull request:
https://github.com/apache/phoenix/pull/229
PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Refer PR #192 for earlier discussion on cursor implementation. As mentioned in the title, this PR implements a sub set of ``cursor`` feature taking into consideration the feedback received in the earlier discussion.
**Note** Thanks to: Gabriel Jimenez <gj...@bloomberg.com>, Ankit Singhal <an...@gmail.com> who had contributed code for this PR.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/bloomberg/phoenix phoenix_cursor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/229.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #229
----
commit 5d0be76eb442296edd814acd565f2ad1c1a3bdcf
Author: Biju Nair <gs...@gmail.com>
Date: 2016-08-16T17:59:52Z
PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Authors: Gabriel Jimenez <gj...@bloomberg.com>, Ankit Singhal <an...@gmail.com>
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on the issue:
https://github.com/apache/phoenix/pull/229
This looks good now, @bijugs - much simpler - nice work. Have you attached a patch in the right format (see https://phoenix.apache.org/contributing.html#Generate_a_patch) to make sure your change doesn't cause any regressions?
@ankitsinghal - WDYT?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r109999101
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java ---
@@ -59,6 +60,8 @@
* Returns projector used to formulate resultSet row
*/
RowProjector getProjector();
+
+ String getCursorName();
--- End diff --
Why do we need to have getCursorName in QueryPlan?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r113032806
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java ---
@@ -59,6 +60,8 @@
* Returns projector used to formulate resultSet row
*/
RowProjector getProjector();
+
+ String getCursorName();
--- End diff --
@JamesRTaylor , will update the PR with suggested changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by ankitsinghal <gi...@git.apache.org>.
Github user ankitsinghal commented on the issue:
https://github.com/apache/phoenix/pull/229
Looks good to me as well. Thanks @bijugs, for working on this. Let me commit this for you.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r109999908
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java ---
@@ -0,0 +1,687 @@
+/*
+ * 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.phoenix.end2end;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.phoenix.util.*;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.math.BigDecimal;
+import java.sql.*;
+import java.util.Properties;
+import java.util.Random;
+
+import static org.apache.phoenix.util.TestUtil.*;
+import static org.junit.Assert.*;
+
+
+public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
--- End diff --
We're trying to get rid of all tests derived from BaseClientManagedTimeIT. Please derive from ParallelStatsDisabledIT instead, letting HBase manage the timestamps, and generate unique table names for each test (see other tests for examples).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs closed the pull request at:
https://github.com/apache/phoenix/pull/229
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r109997991
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java ---
@@ -0,0 +1,75 @@
+/*
+ * 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.phoenix.iterate;
+
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.CursorUtil;
+
+import java.sql.SQLException;
+import java.util.List;
+
+public class CursorResultIterator implements ResultIterator {
+ private String cursorName;
+ private PeekingResultIterator delegate;
+ //TODO Configure fetch size from FETCH call
+ private int fetchSize = 0;
+ private int rowsRead = 0;
+ public CursorResultIterator(PeekingResultIterator delegate, String cursorName) {
+ this.delegate = delegate;
+ this.cursorName = cursorName;
+ }
+
+ @Override
+ public Tuple next() throws SQLException {
+ if(!CursorUtil.moreValues(cursorName)){
+ return null;
+ } else if (fetchSize == rowsRead) {
+ return null;
+ }
+
+ Tuple next = delegate.next();
+ CursorUtil.updateCursor(cursorName,next, delegate.peek());
+ rowsRead++;
+ return next;
+ }
+
+ @Override
+ public void explain(List<String> planSteps) {
+ delegate.explain(planSteps);
+ planSteps.add("CLIENT CURSOR " + cursorName);
+ }
+
+ @Override
+ public String toString() {
+ return "CursorResultIterator [cursor=" + cursorName + "]";
+ }
+
+ @Override
+ public void close() throws SQLException {
+ //NOP
--- End diff --
Does this need to call delegate.close()? If not, what closes the underlying query?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on the issue:
https://github.com/apache/phoenix/pull/229
Attached patch for the changes to the JIRA ticket.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r110000317
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java ---
@@ -0,0 +1,687 @@
+/*
+ * 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.phoenix.end2end;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.phoenix.util.*;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.math.BigDecimal;
+import java.sql.*;
+import java.util.Properties;
+import java.util.Random;
+
+import static org.apache.phoenix.util.TestUtil.*;
+import static org.junit.Assert.*;
+
+
+public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
+ private static final String TABLE_NAME = "CursorRVCTestTable";
+ protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class);
+
+ public void createAndInitializeTestTable() throws SQLException {
+ Connection conn = DriverManager.getConnection(getUrl());
+
+ PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME +
+ "(a_id INTEGER NOT NULL, " +
+ "a_data INTEGER, " +
+ "CONSTRAINT my_pk PRIMARY KEY (a_id))");
+ stmt.execute();
+ synchronized (conn){
+ conn.commit();
+ }
+
+ //Upsert test values into the test table
+ Random rand = new Random();
+ stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME +
+ "(a_id, a_data) VALUES (?,?)");
+ int rowCount = 0;
+ while(rowCount < 100){
+ stmt.setInt(1, rowCount);
+ stmt.setInt(2, rand.nextInt(501));
+ stmt.execute();
+ ++rowCount;
+ }
+ synchronized (conn){
+ conn.commit();
+ }
+ }
+
+ public void deleteTestTable() throws SQLException {
+ Connection conn = DriverManager.getConnection(getUrl());
+ PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME);
+ stmt.execute();
+ synchronized (conn){
+ conn.commit();
+ }
+ }
+
+ @Test
+ public void testCursorsOnTestTablePK() throws SQLException {
+ try{
+ createAndInitializeTestTable();
+ String querySQL = "SELECT a_id FROM " + TABLE_NAME;
+
+ //Test actual cursor implementation
+ String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL;
+ DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
+ cursorSQL = "OPEN testCursor";
+ DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
+ cursorSQL = "FETCH NEXT FROM testCursor";
--- End diff --
Is FETCH PRIOR tested at all? If not supported, should we remove it from the grammar for now?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on the issue:
https://github.com/apache/phoenix/pull/229
@ankitsinghal , @JamesRTaylor , updated the PR with the changes for the latest review comments. If there are any other comments please let me know. Once review is complete will submit a ``diff`` to the JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by ankitsinghal <gi...@git.apache.org>.
Github user ankitsinghal commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r100738871
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/util/CursorUtil.java ---
@@ -0,0 +1,203 @@
+/*
+ * 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.phoenix.util;
+
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.QueryPlan;
+import org.apache.phoenix.compile.OrderByCompiler.OrderBy;
+import org.apache.phoenix.execute.CursorFetchPlan;
+import org.apache.phoenix.iterate.CursorResultIterator;
+import org.apache.phoenix.parse.CloseStatement;
+import org.apache.phoenix.parse.DeclareCursorStatement;
+import org.apache.phoenix.parse.OpenStatement;
+import org.apache.phoenix.schema.tuple.Tuple;
+
+public final class CursorUtil {
+
+ private static class CursorWrapper {
+ private final String cursorName;
+ private final String selectSQL;
+ private boolean isOpen = false;
+ QueryPlan queryPlan;
+ ImmutableBytesWritable row;
+ ImmutableBytesWritable previousRow;
+ private Scan scan;
+ private boolean moreValues=true;
+ private boolean isReversed;
+ private boolean islastCallNext;
+ private CursorFetchPlan fetchPlan;
+ private int offset = -1;
+
+ private CursorWrapper(String cursorName, String selectSQL, QueryPlan queryPlan){
+ this.cursorName = cursorName;
+ this.selectSQL = selectSQL;
+ this.queryPlan = queryPlan;
+ this.islastCallNext = true;
+ this.fetchPlan = new CursorFetchPlan(queryPlan);
+ }
+
+ private synchronized void openCursor(Connection conn) throws SQLException {
+ if(isOpen){
+ return;
+ }
+ this.scan = this.queryPlan.getContext().getScan();
+ isReversed=OrderBy.REV_ROW_KEY_ORDER_BY.equals(this.queryPlan.getOrderBy());
+ isOpen = true;
+ }
+
+ private void closeCursor() throws SQLException {
+ isOpen = false;
+ ((CursorResultIterator) fetchPlan.iterator()).closeCursor();
+ //TODO: Determine if the cursor should be removed from the HashMap at this point.
+ //Semantically it makes sense that something which is 'Closed' one should be able to 'Open' again.
+ mapCursorIDQuery.remove(this.cursorName);
+ }
+
+ private QueryPlan getFetchPlan(boolean isNext, int fetchSize) throws SQLException {
+ if (!isOpen)
+ throw new SQLException("Fetch call on closed cursor '" + this.cursorName + "'!");
+ ((CursorResultIterator)fetchPlan.iterator()).setFetchSize(fetchSize);
+ if (!queryPlan.getStatement().isAggregate() || !queryPlan.getStatement().isDistinct()) {
+ if (islastCallNext != isNext) {
+ if (islastCallNext && !isReversed){
+ ScanUtil.setReversed(scan);
+ } else {
+ ScanUtil.unsetReversed(scan);
+ }
--- End diff --
this code seems to be for reverse/prior and belongs to another JIRA. can we remove this if it can affect the functionality?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r109999394
--- Diff: phoenix-core/src/main/antlr3/PhoenixSQL.g ---
@@ -742,7 +751,26 @@ upsert_column_refs returns [Pair<List<ColumnDef>,List<ColumnName>> ret]
: d=dyn_column_name_or_def { if (d.getDataType()!=null) { $ret.getFirst().add(d); } $ret.getSecond().add(d.getColumnDefName()); }
(COMMA d=dyn_column_name_or_def { if (d.getDataType()!=null) { $ret.getFirst().add(d); } $ret.getSecond().add(d.getColumnDefName()); } )*
;
-
+
+// Parse a full declare cursor expression structure.
+declare_cursor_node returns [DeclareCursorStatement ret]
+ : DECLARE c=cursor_name CURSOR FOR s=select_node
+ {ret = factory.declareCursor(c, s); }
+ ;
+
+cursor_open_node returns [OpenStatement ret]
+ : OPEN c=cursor_name {ret = factory.open(c);}
+ ;
+
+cursor_close_node returns [CloseStatement ret]
+ : CLOSE c=cursor_name {ret = factory.close(c);}
+ ;
+
+cursor_fetch_node returns [FetchStatement ret]
+ : FETCH NEXT (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name {ret = factory.fetch(c,true, a == null ? 1 : Integer.parseInt( a.getText() )); }
+ | FETCH PRIOR (a=NUMBER)? (ROW|ROWS)? FROM c=cursor_name {ret = factory.fetch(c,false, a == null ? 1 : Integer.parseInt( a.getText() )); }
--- End diff --
How is FETCH PRIOR implemented?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r113033881
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java ---
@@ -0,0 +1,687 @@
+/*
+ * 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.phoenix.end2end;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.phoenix.util.*;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.math.BigDecimal;
+import java.sql.*;
+import java.util.Properties;
+import java.util.Random;
+
+import static org.apache.phoenix.util.TestUtil.*;
+import static org.junit.Assert.*;
+
+
+public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
--- End diff --
Will update the code to use ``ParallelStatsDisabledIT ``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r113034318
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/CursorWithRowValueConstructorIT.java ---
@@ -0,0 +1,687 @@
+/*
+ * 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.phoenix.end2end;
+
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.apache.phoenix.util.*;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import java.math.BigDecimal;
+import java.sql.*;
+import java.util.Properties;
+import java.util.Random;
+
+import static org.apache.phoenix.util.TestUtil.*;
+import static org.junit.Assert.*;
+
+
+public class CursorWithRowValueConstructorIT extends BaseClientManagedTimeIT {
+ private static final String TABLE_NAME = "CursorRVCTestTable";
+ protected static final Log LOG = LogFactory.getLog(CursorWithRowValueConstructorIT.class);
+
+ public void createAndInitializeTestTable() throws SQLException {
+ Connection conn = DriverManager.getConnection(getUrl());
+
+ PreparedStatement stmt = conn.prepareStatement("CREATE TABLE IF NOT EXISTS " + TABLE_NAME +
+ "(a_id INTEGER NOT NULL, " +
+ "a_data INTEGER, " +
+ "CONSTRAINT my_pk PRIMARY KEY (a_id))");
+ stmt.execute();
+ synchronized (conn){
+ conn.commit();
+ }
+
+ //Upsert test values into the test table
+ Random rand = new Random();
+ stmt = conn.prepareStatement("UPSERT INTO " + TABLE_NAME +
+ "(a_id, a_data) VALUES (?,?)");
+ int rowCount = 0;
+ while(rowCount < 100){
+ stmt.setInt(1, rowCount);
+ stmt.setInt(2, rand.nextInt(501));
+ stmt.execute();
+ ++rowCount;
+ }
+ synchronized (conn){
+ conn.commit();
+ }
+ }
+
+ public void deleteTestTable() throws SQLException {
+ Connection conn = DriverManager.getConnection(getUrl());
+ PreparedStatement stmt = conn.prepareStatement("DROP TABLE IF EXISTS " + TABLE_NAME);
+ stmt.execute();
+ synchronized (conn){
+ conn.commit();
+ }
+ }
+
+ @Test
+ public void testCursorsOnTestTablePK() throws SQLException {
+ try{
+ createAndInitializeTestTable();
+ String querySQL = "SELECT a_id FROM " + TABLE_NAME;
+
+ //Test actual cursor implementation
+ String cursorSQL = "DECLARE testCursor CURSOR FOR " + querySQL;
+ DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
+ cursorSQL = "OPEN testCursor";
+ DriverManager.getConnection(getUrl()).prepareStatement(cursorSQL).execute();
+ cursorSQL = "FETCH NEXT FROM testCursor";
--- End diff --
Since this PR is for ``FETCH NEXT`` will remove the code related to ``FETCH PRIOR``.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r113032237
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/iterate/CursorResultIterator.java ---
@@ -0,0 +1,75 @@
+/*
+ * 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.phoenix.iterate;
+
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.CursorUtil;
+
+import java.sql.SQLException;
+import java.util.List;
+
+public class CursorResultIterator implements ResultIterator {
+ private String cursorName;
+ private PeekingResultIterator delegate;
+ //TODO Configure fetch size from FETCH call
+ private int fetchSize = 0;
+ private int rowsRead = 0;
+ public CursorResultIterator(PeekingResultIterator delegate, String cursorName) {
+ this.delegate = delegate;
+ this.cursorName = cursorName;
+ }
+
+ @Override
+ public Tuple next() throws SQLException {
+ if(!CursorUtil.moreValues(cursorName)){
+ return null;
+ } else if (fetchSize == rowsRead) {
+ return null;
+ }
+
+ Tuple next = delegate.next();
+ CursorUtil.updateCursor(cursorName,next, delegate.peek());
+ rowsRead++;
+ return next;
+ }
+
+ @Override
+ public void explain(List<String> planSteps) {
+ delegate.explain(planSteps);
+ planSteps.add("CLIENT CURSOR " + cursorName);
+ }
+
+ @Override
+ public String toString() {
+ return "CursorResultIterator [cursor=" + cursorName + "]";
+ }
+
+ @Override
+ public void close() throws SQLException {
+ //NOP
--- End diff --
@JamesRTaylor , closing the cursor will close the underlying query.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by JamesRTaylor <gi...@git.apache.org>.
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r110057029
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryPlan.java ---
@@ -59,6 +60,8 @@
* Returns projector used to formulate resultSet row
*/
RowProjector getProjector();
+
+ String getCursorName();
--- End diff --
I think an alternate implementation would be to have a CursorQueryPlan which wraps another QueryPlan. This would be similar to the way a UnionPlan was implemented. That way you wouldn't need to litter the other plan implementations with logic to wrap them with the CursorResultIterator (which would be brittle if/when new plans are added).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on the issue:
https://github.com/apache/phoenix/pull/229
Closing the PR since the patch got merged. Thanks @ankitsinghal , @JamesRTaylor .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix pull request #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on c...
Posted by ankitsinghal <gi...@git.apache.org>.
Github user ankitsinghal commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/229#discussion_r100738646
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/execute/CursorFetchPlan.java ---
@@ -0,0 +1,87 @@
+package org.apache.phoenix.execute;
+
+import java.sql.ParameterMetaData;
+import java.sql.SQLException;
+import java.util.List;
+import java.util.Set;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.phoenix.compile.ExplainPlan;
+import org.apache.phoenix.compile.GroupByCompiler.GroupBy;
+import org.apache.phoenix.compile.OrderByCompiler.OrderBy;
+import org.apache.phoenix.compile.QueryPlan;
+import org.apache.phoenix.compile.RowProjector;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.iterate.CursorResultIterator;
+import org.apache.phoenix.iterate.ParallelScanGrouper;
+import org.apache.phoenix.iterate.ResultIterator;
+import org.apache.phoenix.jdbc.PhoenixStatement.Operation;
+import org.apache.phoenix.parse.FilterableStatement;
+import org.apache.phoenix.query.KeyRange;
+import org.apache.phoenix.schema.TableRef;
+
+public class CursorFetchPlan extends DelegateQueryPlan {
+
+ //QueryPlan cursorQueryPlan;
+ private CursorResultIterator resultIterator;
+ private int fetchSize;
+
+ public CursorFetchPlan(QueryPlan cursorQueryPlan) {
+ super(cursorQueryPlan);
+ }
+
+
+ @Override
+ public ResultIterator iterator() throws SQLException {
+ // TODO Auto-generated method stub
+ StatementContext context = delegate.getContext();
+ if (resultIterator != null) {
+ return resultIterator;
+ } else {
+ context.getOverallQueryMetrics().startQuery();
+ resultIterator = (CursorResultIterator) delegate.iterator();
+ return resultIterator;
+ }
+ }
+
+ @Override
+ public ResultIterator iterator(ParallelScanGrouper scanGrouper) throws SQLException {
+ // TODO Auto-generated method stub
+ StatementContext context = delegate.getContext();
+ if (resultIterator != null) {
+ return resultIterator;
+ } else {
+ context.getOverallQueryMetrics().startQuery();
+ resultIterator = (CursorResultIterator) delegate.iterator(scanGrouper);
+ return resultIterator;
+ }
+ }
+
+ @Override
+ public ResultIterator iterator(ParallelScanGrouper scanGrouper, Scan scan) throws SQLException {
+ // TODO Auto-generated method stub
--- End diff --
can you merge these iterators as all are doing the same and base class will be calling iterator(ParallelScanGrouper scanGrouper, Scan scan) internally from other overloaded methods with special parameter value.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] phoenix issue #229: PHOENIX-3572 Support FETCH NEXT|n ROWS query on cursor
Posted by bijugs <gi...@git.apache.org>.
Github user bijugs commented on the issue:
https://github.com/apache/phoenix/pull/229
@ankitsinghal, Thanks for the review comments. I have made the changes for the comments. Will rebase the code to a single commit once the review process is complete.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---