You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by "stoty (via GitHub)" <gi...@apache.org> on 2023/05/15 05:33:52 UTC

[GitHub] [phoenix] stoty opened a new pull request, #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

stoty opened a new pull request, #1607:
URL: https://github.com/apache/phoenix/pull/1607

   …inst salted tables need to be more resilient
   
   add test from PHOENIX-4096


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1268973807


##########
phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java:
##########
@@ -155,6 +155,7 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Set<
             keySlots = whereClause.accept(visitor);
 
             if (keySlots == null && (tenantId == null || !table.isMultiTenant()) && table.getViewIndexId() == null && !minOffset.isPresent()) {
+                // FIXME this overwrites salting info in the scanRange

Review Comment:
   which query will be impacted due to the override?



##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelScansCollector.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.List;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.phoenix.compile.QueryPlan;
+
+/**
+ * Stores some state to help build the parallel Scans structure
+ */
+public class ParallelScansCollector {
+
+    private ParallelScanGrouper grouper;
+    private boolean lastScanCrossedRegionBoundary = false;
+    private List<List<Scan>> parallelScans = new ArrayList<>();
+    private List<Scan> lastBatch = new ArrayList<>();

Review Comment:
   nit: `grouper` and `parallelScans` both can be final



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1553535619

   FYI @kadirozde @jpisaac @tkhurana you might be interested, this is resolving the scan boundary problem for salted tables after split/merge.
   


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1581961573

   somehow jenkins didn't trigger new build since May 25, some issues with build maybe.
   
   just triggered a new one [here](https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1607/)


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1643305174

   surprising that end2end tests were not run https://ci-hadoop.apache.org/job/Phoenix/job/Phoenix-PreCommit-GitHub-PR/job/PR-1607/28/testReport/


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198516663


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }
+        ResultSet rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191211' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());
+        rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191217' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");

Review Comment:
   The now generates and checks 100 rows, that should be enough to hit all buckets (statistically).



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1561363897

   All relevant test pass now.
   I think that the FailoverPhoenixConnectionIT is unrelated.
   Can you take a look now @virajjasani @jpisaac @kadirozde @tkhurana ?


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198495896


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }
+        ResultSet rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191211' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());
+        rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191217' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");

Review Comment:
   Random values in tests are fishy.
   We could increase the number of rows, and check that we get back each of them.
   It's a bit tricky, as the nature of hashing makes it hard to make sure that we cover all buckets.
   



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1588557644

   Finally build runs were successful, let me take a look tomorrow.
   
   At high level, IIUC, ParallelScansCollector is going to keep adding scan objects and all places where we use parallel scans, we are going to use this new class. New version of intersectScan is going to include scan boundaries from salt buckets. Is this correct?


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198190984


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");

Review Comment:
   can we also add one non-key column c4?



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }

Review Comment:
   nit:
   ```
           for (int i = 0; i < regions.size() - 1; i += 2) {
               byte[][] regionsToMerge = new byte[2][];
               regionsToMerge[0] = regions.get(i).getEncodedNameAsBytes();
               regionsToMerge[1] = regions.get(i + 1).getEncodedNameAsBytes();
               admin.mergeRegionsAsync(regionsToMerge, false).get();
           }
   ```



##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java:
##########
@@ -1119,12 +1124,21 @@ private List<List<Scan>> getParallelScans(byte[] startKey, byte[] stopKey) throw
                 int gpsComparedToEndKey = -1;
                 boolean everNotDelayed = false;
                 while (intersectWithGuidePosts && (endKey.length == 0 || (gpsComparedToEndKey=currentGuidePost.compareTo(endKey)) <= 0)) {
-                    Scan newScan = scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes, keyOffset,
-                        false);
-                    if (newScan != null) {
-                        ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
-                            regionInfo.getStartKey(), regionInfo.getEndKey(),
-                            newScan.getStartRow(), newScan.getStopRow());
+                    List<Scan> newScans =
+                            scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes,
+                                keyOffset, false, splitPostfix, getTable().getBucketNum());
+                    for (Scan newScan : newScans) {
+                        if (newScan != null) {
+                            ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
+                                regionInfo.getStartKey(), regionInfo.getEndKey(),
+                                newScan.getStartRow(), newScan.getStopRow());
+                            //TODO why are we passing endKey instead of startKey here ?
+                            scans =
+                                    addNewScan(parallelScans, scans, newScan, currentGuidePostBytes,
+                                        false, regionLocation);

Review Comment:
   here `currentGuidePostBytes` could be less than `endKey` right?



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }
+        ResultSet rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191211' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());
+        rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191217' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());

Review Comment:
   For both, shall we also add `assertFalse(rs.next())`?



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1554100173

   Hmm.
   Even with my last change, @jpisaac 's test fail.
   Looking into it.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1560506683

   Pushed fix for salted tables.
   There is still a local index regression I am chasing.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1643352848

   I'm pretty sure that the test were run. 
   We're just aging them out really quick, because the Jenkins workers have very little disk space.
   The final changes will trigger a new CI run.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1583275395

   oops, it seems some tests are taking longer and hence the build is timing out. 
   @stoty do you have any vm or docker setup where you think you could run mvn verify and keep it running for a bit longer? because in this case, we might be able to get better insights from local runs i think.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1644107036

   I took a look at the warning, they are mostly for existing issues.
   I do not plan to fix those, as that would only add more noise to the patch.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1554318685

   I have added @jpisaac 's test, and fixed the error it deteceted.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1268995942


##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/ParallelScansCollector.java:
##########
@@ -0,0 +1,57 @@
+/*
+ * 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 java.util.ArrayList;
+import java.util.List;
+
+import org.apache.hadoop.hbase.client.Scan;
+import org.apache.phoenix.compile.QueryPlan;
+
+/**
+ * Stores some state to help build the parallel Scans structure
+ */
+public class ParallelScansCollector {
+
+    private ParallelScanGrouper grouper;
+    private boolean lastScanCrossedRegionBoundary = false;
+    private List<List<Scan>> parallelScans = new ArrayList<>();
+    private List<Scan> lastBatch = new ArrayList<>();

Review Comment:
   Will fix in a moment



##########
phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java:
##########
@@ -155,6 +155,7 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Set<
             keySlots = whereClause.accept(visitor);
 
             if (keySlots == null && (tenantId == null || !table.isMultiTenant()) && table.getViewIndexId() == null && !minOffset.isPresent()) {
+                // FIXME this overwrites salting info in the scanRange

Review Comment:
   None.
   If this were fixed, we could take use the salting info from the scanRange object.
   Now, we have to go back to the PTable to get it, and the salting info in ScanRange is not used.



##########
phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java:
##########
@@ -155,6 +155,7 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Set<
             keySlots = whereClause.accept(visitor);
 
             if (keySlots == null && (tenantId == null || !table.isMultiTenant()) && table.getViewIndexId() == null && !minOffset.isPresent()) {
+                // FIXME this overwrites salting info in the scanRange

Review Comment:
   Currently none.
   We just need to re-read some salting info from the table, instead of taking the cached value from 



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198516804


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");

Review Comment:
   Done



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }
+        ResultSet rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191211' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());
+        rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191217' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());

Review Comment:
   Done.
   Also added checks to make sure that we get the correct row.



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {

Review Comment:
   I have made the test more exhaustive, to make sure that we hit all buckets.
   We could add a test for splits, but AFAIK there is no problem with splits.
   This specific problem only manifests on merges.



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }

Review Comment:
   Thanks, applied.



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1584151076

   I have attached local-verify-run.gz to the ticket.
   It only fails on the known flakey permission ITs.
   (It seems that we have tests that tend to fail on slow machines, and also ones that tend to fail on fast ones)


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] richardantal commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "richardantal (via GitHub)" <gi...@apache.org>.
richardantal commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1271866038


##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java:
##########
@@ -39,6 +39,7 @@
 import java.io.EOFException;
 import java.sql.SQLException;
 import java.util.*;
+import java.util.Arrays;

Review Comment:
   Can you remove this import please or list everything needed under java.util?



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1271903238


##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java:
##########
@@ -39,6 +39,7 @@
 import java.io.EOFException;
 import java.sql.SQLException;
 import java.util.*;
+import java.util.Arrays;

Review Comment:
   You are right, but this has already been committed.
   Can you open a follow-up ticket ?



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1562749476

   Now it should be really ready.
   The stats=enabled case wasn't tested, and was broken.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1562194053

   Hmm.
   I think my changes are confusing the start and end of region/bucket flags.
   Gonna review and push a fix.
   If you haven't started reviewing yet, hold off for a bit.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1270206801


##########
phoenix-core/src/main/java/org/apache/phoenix/compile/WhereOptimizer.java:
##########
@@ -155,6 +155,7 @@ public static Expression pushKeyExpressionsToScan(StatementContext context, Set<
             keySlots = whereClause.accept(visitor);
 
             if (keySlots == null && (tenantId == null || !table.isMultiTenant()) && table.getViewIndexId() == null && !minOffset.isPresent()) {
+                // FIXME this overwrites salting info in the scanRange

Review Comment:
   got it, sounds good



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1643354697

   I can see that there are also quite a few warnings.
   I will address (some of) those based on the new CI run.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198572887


##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java:
##########
@@ -1119,12 +1124,21 @@ private List<List<Scan>> getParallelScans(byte[] startKey, byte[] stopKey) throw
                 int gpsComparedToEndKey = -1;
                 boolean everNotDelayed = false;
                 while (intersectWithGuidePosts && (endKey.length == 0 || (gpsComparedToEndKey=currentGuidePost.compareTo(endKey)) <= 0)) {
-                    Scan newScan = scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes, keyOffset,
-                        false);
-                    if (newScan != null) {
-                        ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
-                            regionInfo.getStartKey(), regionInfo.getEndKey(),
-                            newScan.getStartRow(), newScan.getStopRow());
+                    List<Scan> newScans =
+                            scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes,
+                                keyOffset, false, splitPostfix, getTable().getBucketNum());
+                    for (Scan newScan : newScans) {
+                        if (newScan != null) {
+                            ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
+                                regionInfo.getStartKey(), regionInfo.getEndKey(),
+                                newScan.getStartRow(), newScan.getStopRow());
+                            //TODO why are we passing endKey instead of startKey here ?
+                            scans =
+                                    addNewScan(parallelScans, scans, newScan, currentGuidePostBytes,
+                                        false, regionLocation);

Review Comment:
   Yes, it could be less than the region's endKey.
   
   I have looked into what the purpose of ParallelScanGrouper.shouldStartNewScan() is.
   
   If we need the scan results to be ordered, then shouldStartNewScan() returns whether the results of the new scan can be concatenated to the results previous scans and keep them ordered (per phoenix SQL semantics)
   
   For salted queries, this checks if the passed in "startKey" is in the same bucket (has the same leading byte) as the startKey of the last scan in the group.
   
   If the salted table is properly pre-split, then it always is, so it doesn't really do anything.
   If the salted table is not properly pre-slit (the very case we are trying to fix), then this makes sure that we put the results for different buckets into different groups.
   
   In either case, it is both an assumption of ParallelScanGrouper and requirement for correct sorting that scans do not pass bucket boundaries, so it doesn't matter whether we check the start or the end key. If the scan passes a bucket boundary, then we have bigger problems than bad result ordering. (i.e., PHOENIX-4096/6910)
   
   I'm not even sure why we even pass key separately, we could just use the startKey of the new region.



##########
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java:
##########
@@ -1119,12 +1124,21 @@ private List<List<Scan>> getParallelScans(byte[] startKey, byte[] stopKey) throw
                 int gpsComparedToEndKey = -1;
                 boolean everNotDelayed = false;
                 while (intersectWithGuidePosts && (endKey.length == 0 || (gpsComparedToEndKey=currentGuidePost.compareTo(endKey)) <= 0)) {
-                    Scan newScan = scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes, keyOffset,
-                        false);
-                    if (newScan != null) {
-                        ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
-                            regionInfo.getStartKey(), regionInfo.getEndKey(),
-                            newScan.getStartRow(), newScan.getStopRow());
+                    List<Scan> newScans =
+                            scanRanges.intersectScan(scan, currentKeyBytes, currentGuidePostBytes,
+                                keyOffset, false, splitPostfix, getTable().getBucketNum());
+                    for (Scan newScan : newScans) {
+                        if (newScan != null) {
+                            ScanUtil.setLocalIndexAttributes(newScan, keyOffset,
+                                regionInfo.getStartKey(), regionInfo.getEndKey(),
+                                newScan.getStartRow(), newScan.getStopRow());
+                            //TODO why are we passing endKey instead of startKey here ?
+                            scans =
+                                    addNewScan(parallelScans, scans, newScan, currentGuidePostBytes,
+                                        false, regionLocation);

Review Comment:
   Thanks for pointing this out, @virajjasani .
   This was actually a bug in my patch: 
   Even if the new code split a scan because of crossing bucket boundaries, we still used the un-split region's key, which meant that we may have grouped different buckets together, which would have broken ordering.
   
   I have removed the endKey parameter, and now use the new scan's end key.
   I have also removed some logic that used the current region's start key as the end key, as it was unneccessary, and incorrect in the merged region case.



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1556536385

   The current version mostly works, but my refactoring has broken query statistics.
   I hope to fix that later today.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1588585813

   Yes, that's correct.
   ParallelScansCollector just simplifies the messy logic where we had to split the lists at the next entry, and maintain the state in object variables.
   I have also consolidated some of the very dispersed logic into fewer places.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty closed pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty closed pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…
URL: https://github.com/apache/phoenix/pull/1607


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1576077776

   Can you review this now that hopefully both cases are working @jpisaac , @virajjasani  ?


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1582593259

   That CI run has hung.
   I'm starting another.
   (We have a number of unresolved flakey tests related to the HA feature)


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] stoty commented on pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#issuecomment-1583918985

   If a test run doesn't finish in 3-4 hours then it never will.
   I've had trouble running tests on either too fast or too slow machines.
   Starting a local test now.


-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


[GitHub] [phoenix] virajjasani commented on a diff in pull request #1607: PHOENIX-6910 Scans created during query compilation and execution aga…

Posted by "virajjasani (via GitHub)" <gi...@apache.org>.
virajjasani commented on code in PR #1607:
URL: https://github.com/apache/phoenix/pull/1607#discussion_r1198227179


##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {
+
+    @Test
+    public void testQuery() throws Exception {
+        String tableName = generateUniqueName();
+
+        Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+        props.put(QueryServices.FORCE_ROW_KEY_ORDER_ATTRIB, Boolean.FALSE.toString());
+        Connection connection = DriverManager.getConnection(getUrl(), props);
+        Statement statement = connection.createStatement();
+        statement.execute("CREATE TABLE " + tableName
+                + " (c1 VARCHAR NOT NULL, c2 VARCHAR NOT NULL, c3 VARCHAR NOT NULL,"
+                + " CONSTRAINT pk PRIMARY KEY(c1,c2,c3)) SALT_BUCKETS=11");
+        statement.execute(
+            " upsert into " + tableName + " values('20191211','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191212','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191213','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191214','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191215','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191216','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191217','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191218','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191219','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191220','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191221','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191222','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191223','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191224','HORTONWORKS_WEEKLY_TEST','v3')");
+        statement.execute(
+            " upsert into " + tableName + " values('20191225','HORTONWORKS_WEEKLY_TEST','v3')");
+        connection.commit();
+
+        Admin admin =
+                driver.getConnectionQueryServices(getUrl(), TestUtil.TEST_PROPERTIES).getAdmin();
+        List<RegionInfo> regions = admin.getRegions(TableName.valueOf(tableName));
+        for (int i = 0; i < regions.size() - 1; i+=2) {
+            admin.mergeRegionsAsync(regions.get(i).getEncodedNameAsBytes(),
+                regions.get(i + 1).getEncodedNameAsBytes(), false).get();
+        }
+        ResultSet rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191211' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");
+        assertTrue(rs.next());
+        rs =
+                statement.executeQuery("select c1,c2,c3 from " + tableName
+                        + " where c1='20191217' and c2 like '%HORTONWORKS_WEEKLY_TEST%'");

Review Comment:
   shall we pick random c1 values from a list of values instead of only with `20191211` and `20191217`?
   
   we can create a list with multiple values and try using UPSERT in a loop against the list, that will reduce num of unique UPSERT statements we need to run in the test and also facilitate querying against any random c1 values from the given list.



##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/salted/SaltedTableMergeBucketsIT.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.salted;
+
+import static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.List;
+import java.util.Properties;
+
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.end2end.ParallelStatsDisabledTest;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.TestUtil;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+@Category(ParallelStatsDisabledTest.class)
+public class SaltedTableMergeBucketsIT extends ParallelStatsDisabledIT {

Review Comment:
   shall we make this `SaltedTableSplitMergeBucketsIT` and add another test that does splitting of salted table region and then verify the result of SELECT queries against randomly picked key values from UPSERT queries?



-- 
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: issues-unsubscribe@phoenix.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org