[GitHub] phoenix issue #320: PHOENIX-4820
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/320 Thanks for the earlier comment, @maryannxue. Do you think this approach is a good first step? Can you think of any cases where this approach will be problematic? ---
[GitHub] phoenix pull request #320: PHOENIX-4820
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/320#discussion_r206717157
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -2951,7 +2951,129 @@ public void testOrderPreservingGroupBy() throws
Exception {
}
}
}
-
+
+@Test
+public void testOrderPreservingGroupByForNotPkColumns() throws
Exception {
+
+try (Connection conn= DriverManager.getConnection(getUrl())) {
+
+conn.createStatement().execute("CREATE TABLE test (\n" +
+"pk1 varchar, \n" +
+"pk2 varchar, \n" +
+"pk3 varchar, \n" +
+"pk4 varchar, \n" +
+"v1 varchar, \n" +
+"v2 varchar,\n" +
+"CONSTRAINT pk PRIMARY KEY (\n" +
+" pk1,\n" +
+" pk2,\n" +
+" pk3,\n" +
+" pk4\n" +
+" )\n" +
+" )");
+String[] queries = new String[] {
+"SELECT pk3 FROM test WHERE v2 = 'a' GROUP BY
substr(v2,0,1),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'c' and v2 =
substr('abc',1,1) GROUP BY v2,pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE v1 = 'a' and v2 = 'b'
GROUP BY length(v1)+length(v2),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and v2 = 'b'
GROUP BY length(pk1)+length(v2),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE v1 = 'a' and v2 =
substr('abc',2,1) GROUP BY pk4,CASE WHEN v1 > v2 THEN v1 ELSE v2 END,pk3 ORDER
BY pk4,pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and v2 =
substr('abc',2,1) GROUP BY pk4,CASE WHEN pk1 > v2 THEN pk1 ELSE v2 END,pk3
ORDER BY pk4,pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and pk2 = 'b'
and v1 = 'c' GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE
pk2 END,pk3 ORDER BY pk3"
+};
+int index = 0;
+for (String query : queries) {
+QueryPlan plan = getQueryPlan(conn, query);
+assertTrue((index + 1) + ") " + queries[index],
plan.getOrderBy().getOrderByExpressions().isEmpty());
+index++;
+}
+}
+}
+
+@Test
+public void testOrderPreservingGroupByForClientAggregatePlan() throws
Exception {
+Connection conn = null;
+ try {
+ conn = DriverManager.getConnection(getUrl());
+ String tableName1 = "test_table";
+ String sql = "create table " + tableName1 + "( "+
+ " pk1 varchar not null , " +
+ " pk2 varchar not null, " +
+ " pk3 varchar not null," +
+ " v1 varchar, " +
+ " v2 varchar, " +
+ " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+"pk1,"+
+"pk2,"+
+"pk3 ))";
+ conn.createStatement().execute(sql);
+
+ String[] queries = new String[] {
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "group by a.ak3,a.av1 order by a.ak3,a.av1",
+
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "where a.av2 = 'a' GROUP BY substr(a.av2,0,1),ak3 ORDER
BY ak3",
+
+ //for InListExpression
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "where a.av2 in('a') GROUP BY substr(a.av2,0,1),ak3
ORDER BY ak3",
--- End diff --
Never mind - just saw your comment in the new visitor and you only want to
optimize this single constant case. Would it make sense to have a negative test
for this below to make sure it doesn't get optimized when there are more than a
single constant in an IN clause?
---
[GitHub] phoenix pull request #320: PHOENIX-4820
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/320#discussion_r206715618
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -2951,7 +2951,129 @@ public void testOrderPreservingGroupBy() throws
Exception {
}
}
}
-
+
+@Test
+public void testOrderPreservingGroupByForNotPkColumns() throws
Exception {
+
+try (Connection conn= DriverManager.getConnection(getUrl())) {
+
+conn.createStatement().execute("CREATE TABLE test (\n" +
+"pk1 varchar, \n" +
+"pk2 varchar, \n" +
+"pk3 varchar, \n" +
+"pk4 varchar, \n" +
+"v1 varchar, \n" +
+"v2 varchar,\n" +
+"CONSTRAINT pk PRIMARY KEY (\n" +
+" pk1,\n" +
+" pk2,\n" +
+" pk3,\n" +
+" pk4\n" +
+" )\n" +
+" )");
+String[] queries = new String[] {
+"SELECT pk3 FROM test WHERE v2 = 'a' GROUP BY
substr(v2,0,1),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'c' and v2 =
substr('abc',1,1) GROUP BY v2,pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE v1 = 'a' and v2 = 'b'
GROUP BY length(v1)+length(v2),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and v2 = 'b'
GROUP BY length(pk1)+length(v2),pk3 ORDER BY pk3",
+"SELECT pk3 FROM test WHERE v1 = 'a' and v2 =
substr('abc',2,1) GROUP BY pk4,CASE WHEN v1 > v2 THEN v1 ELSE v2 END,pk3 ORDER
BY pk4,pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and v2 =
substr('abc',2,1) GROUP BY pk4,CASE WHEN pk1 > v2 THEN pk1 ELSE v2 END,pk3
ORDER BY pk4,pk3",
+"SELECT pk3 FROM test WHERE pk1 = 'a' and pk2 = 'b'
and v1 = 'c' GROUP BY CASE WHEN pk1 > pk2 THEN v1 WHEN pk1 = pk2 THEN pk1 ELSE
pk2 END,pk3 ORDER BY pk3"
+};
+int index = 0;
+for (String query : queries) {
+QueryPlan plan = getQueryPlan(conn, query);
+assertTrue((index + 1) + ") " + queries[index],
plan.getOrderBy().getOrderByExpressions().isEmpty());
+index++;
+}
+}
+}
+
+@Test
+public void testOrderPreservingGroupByForClientAggregatePlan() throws
Exception {
+Connection conn = null;
+ try {
+ conn = DriverManager.getConnection(getUrl());
+ String tableName1 = "test_table";
+ String sql = "create table " + tableName1 + "( "+
+ " pk1 varchar not null , " +
+ " pk2 varchar not null, " +
+ " pk3 varchar not null," +
+ " v1 varchar, " +
+ " v2 varchar, " +
+ " CONSTRAINT TEST_PK PRIMARY KEY ( "+
+"pk1,"+
+"pk2,"+
+"pk3 ))";
+ conn.createStatement().execute(sql);
+
+ String[] queries = new String[] {
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "group by a.ak3,a.av1 order by a.ak3,a.av1",
+
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "where a.av2 = 'a' GROUP BY substr(a.av2,0,1),ak3 ORDER
BY ak3",
+
+ //for InListExpression
+ "select a.ak3 "+
+ "from (select substr(pk1,1,1) ak1,substr(pk2,1,1)
ak2,substr(pk3,1,1) ak3,substr(v1,1,1) av1,substr(v2,1,1) av2 from
"+tableName1+" order by pk2,pk3 limit 10) a "+
+ "where a.av2 in('a') GROUP BY substr(a.av2,0,1),ak3
ORDER BY ak3",
--- End diff --
For the IN test, please add more than one constant (or this gets optimized
to a.av2='a'
---
[GitHub] phoenix issue #298: PHOENIX-4666 Persistent subquery cache for hash joins
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/298 Looks really good to me. Nice work, @ortutay! Any further comments, @maryannxue? ---
[GitHub] phoenix issue #308: Client-side hash aggregation
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/308 +1. Nice work, @geraldss. @twdsilva - would you (or maybe someone else?) have some spare cycles to check this in assuming the test run comes back clean? ---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206328431
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ExplainTable.java ---
@@ -204,14 +205,19 @@ private void appendPKColumnValue(StringBuilder buf,
byte[] range, Boolean isNull
range = ptr.get();
}
if (changeViewIndexId) {
-Short s = (Short) type.toObject(range);
-s = (short) (s + (-Short.MAX_VALUE));
-buf.append(s.toString());
+PDataType viewIndexDataType =
tableRef.getTable().getViewIndexType();
+buf.append(getViewIndexValue(type, range,
viewIndexDataType).toString());
} else {
Format formatter = context.getConnection().getFormatter(type);
buf.append(type.toStringLiteral(range, formatter));
}
}
+
+private Long getViewIndexValue(PDataType type, byte[] range, PDataType
viewIndexDataType){
+boolean useLongViewIndex =
MetaDataUtil.getViewIndexIdDataType().equals(viewIndexDataType);
+Object s = type.toObject(range);
+return (useLongViewIndex ? (Long) s : (Short) s) -
(useLongViewIndex ? Long.MAX_VALUE : Short.MAX_VALUE);
--- End diff --
Shouldn't the argument be all that's necessary? Why is an additional
type argument needed? The type as defined in the schema should be the right
type, no?
---
[GitHub] phoenix pull request #317: PHOENIX-3547 Supporting more number of indices pe...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/317#discussion_r206326382
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1431,11 +1438,42 @@ private PTable getTable(RegionScanner scanner, long
clientTimeStamp, long tableT
// server while holding this lock is a bad idea and likely to
cause contention.
return PTableImpl.makePTable(tenantId, schemaName, tableName,
tableType, indexState, timeStamp, tableSeqNum,
pkName, saltBucketNum, columns, parentSchemaName,
parentTableName, indexes, isImmutableRows, physicalTables, defaultFamilyName,
-viewStatement, disableWAL, multiTenant, storeNulls,
viewType, viewIndexId, indexType,
+viewStatement, disableWAL, multiTenant, storeNulls,
viewType, viewIndexType, viewIndexId, indexType,
rowKeyOrderOptimizable, transactionProvider,
updateCacheFrequency, baseColumnCount,
indexDisableTimestamp, isNamespaceMapped,
autoPartitionSeq, isAppendOnlySchema, storageScheme, encodingScheme, cqCounter,
useStatsForParallelization);
}
+private Long getViewIndexId(Cell[] tableKeyValues, PDataType
viewIndexType) {
+Cell viewIndexIdKv = tableKeyValues[VIEW_INDEX_ID_INDEX];
+return viewIndexIdKv == null ? null :
+decodeViewIndexId(viewIndexIdKv, viewIndexType);
+}
+/**
+ * check the value for {@value USE_LONG_VIEW_INDEX} and if its present
consider viewIndexId as long otherwise
+ * read as short and convert it to long
+ *
+ * @param tableKeyValues
+ * @param viewIndexType
+ * @return
+ */
+private Long decodeViewIndexId(Cell viewIndexIdKv, PDataType
viewIndexType) {
+boolean useLongViewIndex =
MetaDataUtil.getViewIndexIdDataType().equals(viewIndexType);
+ return new Long(
+ useLongViewIndex
+ ?
viewIndexType.getCodec().decodeLong(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(),
SortOrder.getDefault())
+ :
MetaDataUtil.getLegacyViewIndexIdDataType().getCodec().decodeShort(viewIndexIdKv.getValueArray(),
+ viewIndexIdKv.getValueOffset(),
SortOrder.getDefault())
+ );
+}
+
+private PDataType getViewIndexType(Cell[] tableKeyValues) {
+Cell useLongViewIndexKv = tableKeyValues[USE_LONG_VIEW_INDEX];
--- End diff --
I think we need to keep the single VIEW_INDEX_ID column and make sure it's
type is defined (through a property) at create time (and not allow it to be
changed). The issue isn't with the metadata, but with the row key of the rows
of the table. In old tables, it'll be a short while for new tables it'll be a
long. We don't want to have to rewrite the data.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r206295564
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -135,17 +142,25 @@ public ResultIterator iterator(ParallelScanGrouper
scanGrouper, Scan scan) throw
aggResultIterator = new
ClientUngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators);
aggResultIterator = new
UngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
} else {
-if (!groupBy.isOrderPreserving()) {
-int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt(
-QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
-List keyExpressions =
groupBy.getKeyExpressions();
+List keyExpressions = groupBy.getKeyExpressions();
+if (groupBy.isOrderPreserving()) {
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
+} else {
+int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt
+(QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
List keyExpressionOrderBy =
Lists.newArrayListWithExpectedSize(keyExpressions.size());
for (Expression keyExpression : keyExpressions) {
keyExpressionOrderBy.add(new
OrderByExpression(keyExpression, false, true));
}
-iterator = new OrderedResultIterator(iterator,
keyExpressionOrderBy, thresholdBytes, null, null,
projector.getEstimatedRowByteSize());
+
+if (useHashAgg) {
+boolean sort = orderBy == OrderBy.FWD_ROW_KEY_ORDER_BY;
+aggResultIterator = new
ClientHashAggregatingResultIterator(context, iterator, serverAggregators,
keyExpressions, sort);
--- End diff --
Please add comment about reverse sort and point to JIRA (create one if
there's not already one). Might be best to just have the code here such that
once reverse sort is supported, the hash aggregate will just work.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r206295347
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ClientHashAggregateIT.java
---
@@ -0,0 +1,191 @@
+/*
+ * 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 static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.Properties;
+
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.QueryUtil;
+import org.junit.Test;
+
+public class ClientHashAggregateIT extends ParallelStatsDisabledIT {
--- End diff --
Unless I'm missing something, this needs a few more tests:
* verify CLIENT SORTED BY is present in explain plan when sort required for
hash aggregate
* verify it's not in explain plan when sort not required
* verify query results when sort not required
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/314#discussion_r205981788 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/expression/Expression.java --- @@ -88,4 +88,10 @@ * @return */ boolean requiresFinalEvaluation(); + +/** + * + * @return + */ +boolean isConstantIfChildrenAllConstant(); --- End diff -- It's not clear that we need this. We already rollup determinism and isStateless for the expression tree. If determinism == Determinism.PER_STATEMENT or Determinism.ALWAYS and isStateless is true, then we know an expression is a constant. We have a utility for this in ExpressionUtil.isConstant(Expression). ---
[GitHub] phoenix pull request #314: PHOENIX-4820
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/314#discussion_r205981903
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/util/ExpressionUtil.java ---
@@ -68,4 +74,85 @@ public static boolean isPkPositionChanging(TableRef
tableRef, List p
return false;
}
+public static boolean isColumnConstant(Expression columnExpression,
Expression whereExpression) {
+if(whereExpression == null) {
+return false;
+}
+IsColumnConstantExpressionVisitor
isColumnConstantExpressionVisitor =
+new IsColumnConstantExpressionVisitor(columnExpression);
+whereExpression.accept(isColumnConstantExpressionVisitor);
+return isColumnConstantExpressionVisitor.isConstant();
+}
+
+private static class IsColumnConstantExpressionVisitor extends
StatelessTraverseNoExpressionVisitor {
--- End diff --
I'm hoping we don't need a new visitor here. There are probably other cases
to check for besides ComparisonExpression. For example, InListExpression, maybe
CoerceExpression, etc.
---
[GitHub] phoenix pull request #314: PHOENIX-4820
Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/314#discussion_r205982004 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java --- @@ -559,8 +559,16 @@ protected QueryPlan compileSingleFlatQuery(StatementContext context, SelectState groupBy = groupBy.compile(context, innerPlanTupleProjector); context.setResolver(resolver); // recover resolver RowProjector projector = ProjectionCompiler.compile(context, select, groupBy, asSubquery ? Collections.emptyList() : targetColumns, where); -OrderBy orderBy = OrderByCompiler.compile(context, select, groupBy, limit, offset, projector, -groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, isInRowKeyOrder); +OrderBy orderBy = OrderByCompiler.compile( +context, +select, +groupBy, +limit, +offset, +projector, +groupBy == GroupBy.EMPTY_GROUP_BY ? innerPlanTupleProjector : null, +groupBy == GroupBy.EMPTY_GROUP_BY ? isInRowKeyOrder : true, +where); --- End diff -- Would be ideal if the change could be isolated to OrderByCompiler being aware of its inner plan to determine correctly whether the OrderBy can be compiled out. Any ideas, @maryannxue? ---
[GitHub] phoenix issue #308: Client-side hash aggregation
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/308 Looks like the move of 5.x branch to become master messed up this PR. Can you please start a new pull request against the 4.x-HBase-1.4 branch (this is what was master before), @geraldss? FYI, the last step will be to get a test run with your patch in place to make sure there are no regressions. To do that, attach a .patch file to the JIRA (include the branch name in the name of the patch file) and click on the Submit Patch button. See http://phoenix.apache.org/contributing.html#Generate_a_patch for more info on that. ---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r205232207
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -183,13 +198,15 @@ public ExplainPlan getExplainPlan() throws
SQLException {
if (where != null) {
planSteps.add("CLIENT FILTER BY " + where.toString());
}
-if (!groupBy.isEmpty()) {
-if (!groupBy.isOrderPreserving()) {
-planSteps.add("CLIENT SORTED BY " +
groupBy.getKeyExpressions().toString());
-}
+if (groupBy.isEmpty()) {
+planSteps.add("CLIENT AGGREGATE INTO SINGLE ROW");
+} else if (groupBy.isOrderPreserving()) {
planSteps.add("CLIENT AGGREGATE INTO DISTINCT ROWS BY " +
groupBy.getExpressions().toString());
+} else if (useHashAgg) {
+planSteps.add("CLIENT HASH AGGREGATE INTO DISTINCT ROWS BY " +
groupBy.getExpressions().toString());
--- End diff --
Add CLIENT SORTED BY line here if sorting required for hash aggregate.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r205229353
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ClientHashAggregateIT.java
---
@@ -0,0 +1,191 @@
+/*
+ * 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 static org.apache.phoenix.util.TestUtil.TEST_PROPERTIES;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.Statement;
+import java.util.Properties;
+
+import org.apache.phoenix.util.PropertiesUtil;
+import org.apache.phoenix.util.QueryUtil;
+import org.junit.Test;
+
+public class ClientHashAggregateIT extends ParallelStatsDisabledIT {
--- End diff --
Add tests for when sort (forward & reverse) required for hash aggregate.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r205231380
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -135,17 +142,25 @@ public ResultIterator iterator(ParallelScanGrouper
scanGrouper, Scan scan) throw
aggResultIterator = new
ClientUngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators);
aggResultIterator = new
UngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
} else {
-if (!groupBy.isOrderPreserving()) {
-int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt(
-QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
-List keyExpressions =
groupBy.getKeyExpressions();
+List keyExpressions = groupBy.getKeyExpressions();
+if (groupBy.isOrderPreserving()) {
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
+} else {
+int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt
+(QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
List keyExpressionOrderBy =
Lists.newArrayListWithExpectedSize(keyExpressions.size());
for (Expression keyExpression : keyExpressions) {
keyExpressionOrderBy.add(new
OrderByExpression(keyExpression, false, true));
}
-iterator = new OrderedResultIterator(iterator,
keyExpressionOrderBy, thresholdBytes, null, null,
projector.getEstimatedRowByteSize());
+
+if (useHashAgg) {
+boolean sort = orderBy == OrderBy.FWD_ROW_KEY_ORDER_BY;
+aggResultIterator = new
ClientHashAggregatingResultIterator(context, iterator, serverAggregators,
keyExpressions, sort);
--- End diff --
Youâll need to pass through if forward or reverse scan. You might just
pass through orderBy instead of the boolean. Better still would be to let the
code below insert an Ordering result iterator so you wouldnât need the sort
logic at all in your new iterator.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r205230186
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -135,17 +142,25 @@ public ResultIterator iterator(ParallelScanGrouper
scanGrouper, Scan scan) throw
aggResultIterator = new
ClientUngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators);
aggResultIterator = new
UngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
} else {
-if (!groupBy.isOrderPreserving()) {
-int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt(
-QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
-List keyExpressions =
groupBy.getKeyExpressions();
+List keyExpressions = groupBy.getKeyExpressions();
+if (groupBy.isOrderPreserving()) {
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
+} else {
+int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt
+(QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
List keyExpressionOrderBy =
Lists.newArrayListWithExpectedSize(keyExpressions.size());
for (Expression keyExpression : keyExpressions) {
keyExpressionOrderBy.add(new
OrderByExpression(keyExpression, false, true));
}
-iterator = new OrderedResultIterator(iterator,
keyExpressionOrderBy, thresholdBytes, null, null,
projector.getEstimatedRowByteSize());
+
+if (useHashAgg) {
+boolean sort = orderBy == OrderBy.FWD_ROW_KEY_ORDER_BY;
--- End diff --
Should be true if OrderBy.RVS_ROW_KEY_ORDER_BY too.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204197328
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -135,17 +142,24 @@ public ResultIterator iterator(ParallelScanGrouper
scanGrouper, Scan scan) throw
aggResultIterator = new
ClientUngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators);
aggResultIterator = new
UngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
} else {
-if (!groupBy.isOrderPreserving()) {
-int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt(
-QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
-List keyExpressions =
groupBy.getKeyExpressions();
+List keyExpressions = groupBy.getKeyExpressions();
+if (groupBy.isOrderPreserving()) {
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
+} else {
+int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt
+(QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
List keyExpressionOrderBy =
Lists.newArrayListWithExpectedSize(keyExpressions.size());
for (Expression keyExpression : keyExpressions) {
keyExpressionOrderBy.add(new
OrderByExpression(keyExpression, false, true));
}
-iterator = new OrderedResultIterator(iterator,
keyExpressionOrderBy, thresholdBytes, null, null,
projector.getEstimatedRowByteSize());
+
+if (useHashAgg) {
+aggResultIterator = new
ClientHashAggregatingResultIterator(context, iterator, serverAggregators,
keyExpressions);
+} else {
+iterator = new OrderedResultIterator(iterator,
keyExpressionOrderBy, thresholdBytes, null, null,
projector.getEstimatedRowByteSize());
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
+}
}
-aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, groupBy.getKeyExpressions());
aggResultIterator = new
GroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
}
--- End diff --
In the below if statement, you should still insert the
OrderedAggregatingResultIterator if a hash aggregation is being done and
orderBy != OrderBy.EMPTY_ORDER_BY:
// Still sort the aggregated rows if we're using a hash aggregation and
the order by was optimized out
// since the rows won't be in GROUP BY key order
if (orderBy.getOrderByExpressions().isEmpty() && (!useHashAgg ||
orderBy != OrderBy.EMPTY_ORDER_BY)) {
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204196968
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,189 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.SizedUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final int CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private final MemoryChunk memoryChunk;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(StatementContext context,
ResultIterator resultIterator, Aggregators aggregators,
+ List
groupByExpressions, int thresholdBytes) {
+
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+memoryChunk =
context.getConnection().getQueryServices().getMemoryManager().allocate(CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE);
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
--- End diff --
See my comment in ClientAggregatePlan.java. I believe you can detect this
corner case there and not need this sort. It's a little weird to have a hash
aggregation that still does a sort (but I get your point about it being better
than doing the sort before).
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204167291
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,189 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.SizedUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final int CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private final MemoryChunk memoryChunk;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(StatementContext context,
ResultIterator resultIterator, Aggregators aggregators,
+ List
groupByExpressions, int thresholdBytes) {
+
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+memoryChunk =
context.getConnection().getQueryServices().getMemoryManager().allocate(CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE);
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204161104
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,189 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.SizedUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final int CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private final MemoryChunk memoryChunk;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(StatementContext context,
ResultIterator resultIterator, Aggregators aggregators,
+ List
groupByExpressions, int thresholdBytes) {
+
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+memoryChunk =
context.getConnection().getQueryServices().getMemoryManager().allocate(CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE);
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204176238
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,189 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.SizedUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final int CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private final MemoryChunk memoryChunk;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(StatementContext context,
ResultIterator resultIterator, Aggregators aggregators,
+ List
groupByExpressions, int thresholdBytes) {
+
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+memoryChunk =
context.getConnection().getQueryServices().getMemoryManager().allocate(CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE);
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
--- End diff --
I don't think this sort is required at all. You've already guaranteed that
each row has a unique row key, so the subsequent
GroupedAggregatingResultIterator should work fine (that expects duplicate rows
to be adjacent and since every row is unique, that'll be the case). If there's
an ORDER BY for the groups, the Phoenix will insert an ordering result iterator.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r204168576
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,189 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.compile.StatementContext;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.memory.MemoryManager.MemoryChunk;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.SizedUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final int CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private final MemoryChunk memoryChunk;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(StatementContext context,
ResultIterator resultIterator, Aggregators aggregators,
+ List
groupByExpressions, int thresholdBytes) {
+
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+memoryChunk =
context.getConnection().getQueryServices().getMemoryManager().allocate(CLIENT_HASH_AGG_MEMORY_CHUNK_SIZE);
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r203867850
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,173 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(ResultIterator
resultIterator, Aggregators aggregators, List groupByExpressions,
int thresholdBytes) {
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
+hash = null;
+resultIterator.close();
+}
+
+@Override
+public Aggregator[] aggregate(Tuple result) {
+Aggregator[] rowAggregators = aggregators.getAggregators();
+aggregators.reset(rowAggregators);
+aggregators.aggregate(rowAggregators, result);
+return rowAggregators;
+}
+
+@Override
+public void explain(List planSteps) {
+resultIterator.explain(planSteps);
+}
+
+@Override
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r203882988
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashSizeException.java
---
@@ -0,0 +1,33 @@
+/*
+ * 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;
+
+/**
+ * Thrown by {@link
org.apache.phoenix.iterate.ClientHashAggregatingResultIterator } when
+ * hash size exceeds memory threshold.
+ *
+ */
+public class ClientHashSizeException extends RuntimeException {
--- End diff --
You won't need this as an InsufficientMemoryException will be thrown if you
go above the specified memory limit (based on existing Phoenix config
properties) and this will be unwound to become a SQLException with the code
SQLExceptionCode.INSUFFICIENT_MEMORY.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r203868079
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,173 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private final int thresholdBytes;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(ResultIterator
resultIterator, Aggregators aggregators, List groupByExpressions,
int thresholdBytes) {
+Objects.requireNonNull(resultIterator);
+Objects.requireNonNull(aggregators);
+Objects.requireNonNull(groupByExpressions);
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+this.thresholdBytes = thresholdBytes;
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+hash = populateHash();
+keyList = sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
+hash = null;
+resultIterator.close();
+}
+
+@Override
+public Aggregator[] aggregate(Tuple result) {
+Aggregator[] rowAggregators = aggregators.getAggregators();
+aggregators.reset(rowAggregators);
+aggregators.aggregate(rowAggregators, result);
+return rowAggregators;
+}
+
+@Override
+public void explain(List planSteps) {
+resultIterator.explain(planSteps);
+}
+
+@Override
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r203879864
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/execute/ClientAggregatePlan.java
---
@@ -135,17 +142,24 @@ public ResultIterator iterator(ParallelScanGrouper
scanGrouper, Scan scan) throw
aggResultIterator = new
ClientUngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators);
aggResultIterator = new
UngroupedAggregatingResultIterator(LookAheadResultIterator.wrap(aggResultIterator),
clientAggregators);
} else {
-if (!groupBy.isOrderPreserving()) {
-int thresholdBytes =
context.getConnection().getQueryServices().getProps().getInt(
-QueryServices.SPOOL_THRESHOLD_BYTES_ATTRIB,
QueryServicesOptions.DEFAULT_SPOOL_THRESHOLD_BYTES);
-List keyExpressions =
groupBy.getKeyExpressions();
+List keyExpressions = groupBy.getKeyExpressions();
+if (groupBy.isOrderPreserving()) {
+aggResultIterator = new
ClientGroupedAggregatingResultIterator(LookAheadResultIterator.wrap(iterator),
serverAggregators, keyExpressions);
--- End diff --
Pass through context here too to ClientGroupedAggregatingResultIterator as
you'll need it to get the memory manager.
---
[GitHub] phoenix pull request #308: Client-side hash aggregation
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/308#discussion_r202842452
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/ClientHashAggregatingResultIterator.java
---
@@ -0,0 +1,155 @@
+/*
+ * 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 static org.apache.phoenix.query.QueryConstants.AGG_TIMESTAMP;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN;
+import static org.apache.phoenix.query.QueryConstants.SINGLE_COLUMN_FAMILY;
+
+import java.io.IOException;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Map;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+
+import org.apache.hadoop.hbase.Cell;
+import org.apache.hadoop.hbase.KeyValue;
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.expression.aggregator.Aggregator;
+import org.apache.phoenix.expression.aggregator.Aggregators;
+import org.apache.phoenix.schema.tuple.MultiKeyValueTuple;
+import org.apache.phoenix.schema.tuple.Tuple;
+import org.apache.phoenix.util.KeyValueUtil;
+import org.apache.phoenix.util.TupleUtil;
+
+/**
+ *
+ * This class implements client-side hash aggregation in memory.
+ * Issue https://issues.apache.org/jira/browse/PHOENIX-4751.
+ *
+ */
+public class ClientHashAggregatingResultIterator
+implements AggregatingResultIterator {
+
+private static final int HASH_AGG_INIT_SIZE = 64*1024;
+private static final byte[] UNITIALIZED_KEY_BUFFER = new byte[0];
+private final ResultIterator resultIterator;
+private final Aggregators aggregators;
+private final List groupByExpressions;
+private HashMap hash;
+private List keyList;
+private Iterator keyIterator;
+
+public ClientHashAggregatingResultIterator(ResultIterator
resultIterator, Aggregators aggregators, List groupByExpressions) {
+if (resultIterator == null) throw new NullPointerException();
+if (aggregators == null) throw new NullPointerException();
+if (groupByExpressions == null) throw new NullPointerException();
+this.resultIterator = resultIterator;
+this.aggregators = aggregators;
+this.groupByExpressions = groupByExpressions;
+}
+
+@Override
+public Tuple next() throws SQLException {
+if (keyIterator == null) {
+populateHash();
+sortKeys();
+keyIterator = keyList.iterator();
+}
+
+if (!keyIterator.hasNext()) {
+return null;
+}
+
+ImmutableBytesWritable key = keyIterator.next();
+Aggregator[] rowAggregators = hash.get(key);
+byte[] value = aggregators.toBytes(rowAggregators);
+Tuple tuple = wrapKeyValueAsResult(KeyValueUtil.newKeyValue(key,
SINGLE_COLUMN_FAMILY, SINGLE_COLUMN, AGG_TIMESTAMP, value, 0, value.length));
+return tuple;
+}
+
+@Override
+public void close() throws SQLException {
+keyIterator = null;
+keyList = null;
+hash = null;
+resultIterator.close();
+}
+
+@Override
+public Aggregator[] aggregate(Tuple result) {
+Aggregator[] rowAggregators = aggregators.getAggregators();
+aggregators.reset(rowAggregators);
+aggregators.aggregate(rowAggregators, result);
+return rowAggregators;
+}
+
+@Override
+public void explain(List planSteps) {
+resultIterator.explain(planSteps);
+}
+
+@Override
+public String toString() {
+return "ClientHashAggregatingResul
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200208444
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java ---
@@ -1893,26 +1981,45 @@ public static void upgradeTable(PhoenixConnection
conn, String srcTable) throws
conn.commit();
}
conn.getQueryServices().clearTableFromCache(
-conn.getTenantId() == null ?
ByteUtil.EMPTY_BYTE_ARRAY : conn.getTenantId().getBytes(),
+tenantIdBytes,
index.getSchemaName().getBytes(),
index.getTableName().getBytes(),
PhoenixRuntime.getCurrentScn(readOnlyProps));
}
updateIndexesSequenceIfPresent(conn, table);
conn.commit();
-
} else {
throw new RuntimeException("Error: problem occured during
upgrade. Table is not upgraded successfully");
}
if (table.getType() == PTableType.VIEW) {
logger.info(String.format("Updating link information for
view '%s' ..", table.getTableName()));
updateLink(conn, oldPhysicalName,
newPhysicalTablename,table.getSchemaName(),table.getTableName());
conn.commit();
-
+
+// if the view is a first level child, then we need to
create the PARENT_TABLE link
+// that was overwritten by the PHYSICAL_TABLE link
--- End diff --
Ah, good. So we'll be consistent with the parent link now, right?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/303#discussion_r200208319 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java --- @@ -1779,13 +2174,65 @@ public void createTable(RpcController controller, CreateTableRequest request, } } +// The mutations to create a table are written in the following order: +// 1. Write the child link as if the next two steps fail we +// ignore missing children while processing a parent +// 2. Update the encoded column qualifier for the parent table if its on a +// different region server (for tables that use column qualifier encoding) +// if the next step fails we end up wasting a few col qualifiers +// 3. Finally write the mutations to create the table + +// From 4.15 the parent->child links are stored in a separate table SYSTEM.CHILD_LINK +List childLinkMutations = MetaDataUtil.removeChildLinks(tableMetadata); --- End diff -- TODO to remove this code in 4.16. ---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200209126
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java
---
@@ -372,6 +378,31 @@ public void testViewAndTableAndDrop() throws Exception
{
// drop table cascade should succeed
conn.createStatement().execute("DROP TABLE " + fullTableName + "
CASCADE");
+validateViewDoesNotExist(conn, fullViewName1);
+validateViewDoesNotExist(conn, fullViewName2);
+
+}
+
+@Test
+public void testRecreateDroppedTableWithChildViews() throws Exception {
--- End diff --
These new tests are good. These are testing that the left over metadata
doesn't impact the re-creation of a table since we don't make the RPC to delete
views when a base table is dropped, right? Do you think there'd be any issues
if part of the rows for a view were there (i.e. say that the create view
failed, but some of the rows were written)? Might be good to have a test like
this - you could set it up by using HBase APIs to manually delete some rows of
a view.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206862
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1809,180 +2256,97 @@ public void createTable(RpcController controller,
CreateTableRequest request,
} catch (Throwable t) {
logger.error("createTable failed", t);
ProtobufUtil.setControllerException(controller,
-
ServerUtil.createIOException(SchemaUtil.getTableName(schemaName, tableName),
t));
+ServerUtil.createIOException(fullTableName, t));
}
}
+ private void dropChildMetadata(byte[] schemaName, byte[] tableName,
byte[] tenantIdBytes)
+ throws IOException, SQLException,
ClassNotFoundException {
+ TableViewFinderResult childViewsResult = new
TableViewFinderResult();
+ findAllChildViews(tenantIdBytes, schemaName, tableName,
childViewsResult);
+ if (childViewsResult.hasViews()) {
+ for (TableInfo viewInfo :
childViewsResult.getResults()) {
+ byte[] viewTenantId = viewInfo.getTenantId();
+ byte[] viewSchemaName =
viewInfo.getSchemaName();
+ byte[] viewName = viewInfo.getTableName();
+ Properties props = new Properties();
+ if (viewTenantId != null && viewTenantId.length
!= 0)
+
props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB,
Bytes.toString(viewTenantId));
+ try (PhoenixConnection connection =
QueryUtil.getConnectionOnServer(env.getConfiguration())
+
.unwrap(PhoenixConnection.class)) {
+ MetaDataClient client = new
MetaDataClient(connection);
+ org.apache.phoenix.parse.TableName
viewTableName = org.apache.phoenix.parse.TableName
+
.create(Bytes.toString(viewSchemaName), Bytes.toString(viewName));
+ client.dropTable(
+ new
DropTableStatement(viewTableName, PTableType.VIEW, false, true, true));
+ }
+ }
+ }
+ }
+
private boolean execeededIndexQuota(PTableType tableType, PTable
parentTable) {
return PTableType.INDEX == tableType &&
parentTable.getIndexes().size() >= maxIndexesPerTable;
}
-
-private void findAllChildViews(Region region, byte[] tenantId, PTable
table,
-TableViewFinder result, long clientTimeStamp, int
clientVersion) throws IOException, SQLException {
-TableViewFinder currResult = findChildViews(region, tenantId,
table, clientVersion, false);
-result.addResult(currResult);
-for (ViewInfo viewInfo : currResult.getViewInfoList()) {
-byte[] viewtenantId = viewInfo.getTenantId();
-byte[] viewSchema = viewInfo.getSchemaName();
-byte[] viewTable = viewInfo.getViewName();
-byte[] tableKey = SchemaUtil.getTableKey(viewtenantId,
viewSchema, viewTable);
-ImmutableBytesPtr cacheKey = new ImmutableBytesPtr(tableKey);
-PTable view = loadTable(env, tableKey, cacheKey,
clientTimeStamp, clientTimeStamp, clientVersion);
-if (view == null) {
-logger.warn("Found orphan tenant view row in
SYSTEM.CATALOG with tenantId:"
-+ Bytes.toString(tenantId) + ", schema:"
-+ Bytes.toString(viewSchema) + ", table:"
-+ Bytes.toString(viewTable));
-continue;
-}
-findAllChildViews(region, viewtenantId, view, result,
clientTimeStamp, clientVersion);
-}
-}
-
-// TODO use child link instead once splittable system catalog
(PHOENIX-3534) is implemented
-// and we have a separate table for links.
-private TableViewFinder findChildViews_deprecated(Region region,
byte[] tenantId, PTable table, byte[] linkTypeBytes, boolean stopAfterFirst)
throws IOException {
-byte[] schemaName = table.getSchemaName().getBytes();
-byte[] tableName = table.getTableName().getBytes();
-boolean isMultiTenant = table.isMultiTenant();
-Scan scan = new Scan();
-// If the table is multi-tenant, we need to check across all
tenant_ids,
-// so we can't constrain the row key. Ot
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200208028
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -447,7 +447,7 @@
static {
Collections.sort(FUNCTION_KV_COLUMNS, KeyValue.COMPARATOR);
}
-
+
--- End diff --
Might be good to include a class level comment that explains the overall
approach at a high level.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200208160
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/schema/TableProperty.java ---
@@ -231,36 +231,26 @@ public Object getPTableValue(PTable table) {
private final SQLExceptionCode mutatingImmutablePropException;
private final boolean isValidOnView;
private final boolean isMutableOnView;
-private final boolean propagateToViews;
private TableProperty(String propertyName, boolean isMutable, boolean
isValidOnView, boolean isMutableOnView) {
-this(propertyName, COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY,
isMutable, CANNOT_ALTER_PROPERTY, isValidOnView, isMutableOnView, true);
-}
-
-private TableProperty(String propertyName, boolean isMutable, boolean
isValidOnView, boolean isMutableOnView, boolean propagateToViews) {
-this(propertyName, COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY,
isMutable, CANNOT_ALTER_PROPERTY, isValidOnView, isMutableOnView,
propagateToViews);
+this(propertyName, COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY,
isMutable, CANNOT_ALTER_PROPERTY, isValidOnView, isMutableOnView);
}
private TableProperty(String propertyName, SQLExceptionCode
colFamilySpecifiedException, boolean isMutable, boolean isValidOnView, boolean
isMutableOnView) {
-this(propertyName, colFamilySpecifiedException, isMutable,
CANNOT_ALTER_PROPERTY, isValidOnView, isMutableOnView, true);
+this(propertyName, colFamilySpecifiedException, isMutable,
CANNOT_ALTER_PROPERTY, isValidOnView, isMutableOnView);
}
private TableProperty(String propertyName, boolean isMutable, boolean
isValidOnView, boolean isMutableOnView, SQLExceptionCode isMutatingException) {
-this(propertyName, COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY,
isMutable, isMutatingException, isValidOnView, isMutableOnView, true);
+this(propertyName, COLUMN_FAMILY_NOT_ALLOWED_TABLE_PROPERTY,
isMutable, isMutatingException, isValidOnView, isMutableOnView);
}
private TableProperty(String propertyName, SQLExceptionCode
colFamSpecifiedException, boolean isMutable, SQLExceptionCode
mutatingException, boolean isValidOnView, boolean isMutableOnView) {
-this(propertyName, colFamSpecifiedException, isMutable,
mutatingException, isValidOnView, isMutableOnView, true);
-}
-
-private TableProperty(String propertyName, SQLExceptionCode
colFamSpecifiedException, boolean isMutable, SQLExceptionCode
mutatingException, boolean isValidOnView, boolean isMutableOnView, boolean
propagateToViews) {
--- End diff --
How did you end up dealing with table property conflicts between parent and
children? Is there follow up work required? Can we use the timestamp of the
Cell storing the property to differentiate similar to the logic for columns?
It's fine to do this work in a follow up JIRA.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200207759
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/schema/MetaDataClient.java ---
@@ -1957,6 +1968,17 @@ private PTable
createTableInternal(CreateTableStatement statement, byte[][] spli
linkStatement.setLong(6, parent.getSequenceNumber());
linkStatement.setString(7,
PTableType.INDEX.getSerializedValue());
linkStatement.execute();
+
+// Add row linking index table to parent table for indexes
on views
+if (parent.getType() == PTableType.VIEW) {
+ linkStatement =
connection.prepareStatement(CREATE_VIEW_INDEX_PARENT_LINK);
+ linkStatement.setString(1, tenantIdStr);
+ linkStatement.setString(2, schemaName);
+ linkStatement.setString(3, tableName);
+ linkStatement.setString(4,
parent.getName().getString());
+ linkStatement.setByte(5,
LinkType.VIEW_INDEX_PARENT_TABLE.getSerializedValue());
+ linkStatement.execute();
+}
--- End diff --
We need to update MetaDataClient.createTableInternal() to not include the
columns from the parent table in 4.15 so that we can remove the code in
MetaDataEndPointImpl that filters the columns. It's fine to do this in a follow
up JIRA, but we should remember to do it.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200207594
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -3642,30 +3596,62 @@ private void dropIndexes(PTable table, Region
region, List in
boolean isCoveredColumn =
indexMaintainer.getCoveredColumns().contains(colDropRef);
// If index requires this column for its pk, then drop it
if (isColumnIndexed) {
-// Since we're dropping the index, lock it to ensure
-// that a change in index state doesn't
-// occur while we're dropping it.
-acquireLock(region, indexKey, locks);
// Drop the index table. The doDropTable will expand
// this to all of the table rows and invalidate the
// index table
-additionalTableMetaData.add(new Delete(indexKey,
clientTimeStamp));
+Delete delete = new Delete(indexKey, clientTimeStamp);
byte[] linkKey =
MetaDataUtil.getParentLinkKey(tenantId,
schemaName, tableName, index
.getTableName().getBytes());
-// Drop the link between the data table and the
+// Drop the link between the parent table and the
// index table
-additionalTableMetaData.add(new Delete(linkKey,
clientTimeStamp));
-doDropTable(indexKey, tenantId,
index.getSchemaName().getBytes(), index
-.getTableName().getBytes(), tableName,
index.getType(),
-additionalTableMetaData, invalidateList, locks,
tableNamesToDelete, sharedTablesToDelete, false, clientVersion);
-invalidateList.add(new ImmutableBytesPtr(indexKey));
+Delete linkDelete = new Delete(linkKey, clientTimeStamp);
+List tableMetaData =
Lists.newArrayListWithExpectedSize(2);
+Delete tableDelete = delete;
+tableMetaData.add(tableDelete);
+tableMetaData.add(linkDelete);
+// if the index is not present on the current region make
an rpc to drop it
--- End diff --
Is this ever the case since the index should be in the same schema as it's
table? Or is there a corner case with indexes on views?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200207388
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -2573,307 +2897,139 @@ else if (pkCount <= COLUMN_NAME_INDEX
return new
MetaDataMutationResult(MutationCode.UNALLOWED_TABLE_MUTATION,
EnvironmentEdgeManager.currentTimeMillis(), basePhysicalTable);
}
-ColumnOrdinalPositionUpdateList ordinalPositionList = new
ColumnOrdinalPositionUpdateList();
+//add the new columns to the child view
List viewPkCols = new
ArrayList<>(view.getPKColumns());
boolean addingExistingPkCol = false;
-int numCols = view.getColumns().size();
-// add the new columns to the child view
-for (PutWithOrdinalPosition p : columnPutsForBaseTable) {
-Put baseTableColumnPut = p.put;
+for (Put columnToBeAdded : columnPutsForBaseTable) {
PColumn existingViewColumn = null;
byte[][] rkmd = new byte[5][];
-getVarChars(baseTableColumnPut.getRow(), rkmd);
+getVarChars(columnToBeAdded.getRow(), rkmd);
String columnName =
Bytes.toString(rkmd[COLUMN_NAME_INDEX]);
-String columnFamily = rkmd[FAMILY_NAME_INDEX] == null ?
null : Bytes.toString(rkmd[FAMILY_NAME_INDEX]);
+String columnFamily =
+rkmd[FAMILY_NAME_INDEX] == null ? null
+: Bytes.toString(rkmd[FAMILY_NAME_INDEX]);
try {
-existingViewColumn = columnFamily == null ?
view.getColumnForColumnName(columnName) : view.getColumnFamily(
-
columnFamily).getPColumnForColumnName(columnName);
+existingViewColumn =
+columnFamily == null ?
view.getColumnForColumnName(columnName)
+: view.getColumnFamily(columnFamily)
+
.getPColumnForColumnName(columnName);
} catch (ColumnFamilyNotFoundException e) {
-// ignore since it means that the column family is not
present for the column to be added.
+// ignore since it means that the column family is not
present for the column to
+// be added.
} catch (ColumnNotFoundException e) {
// ignore since it means the column is not present in
the view
}
-
-boolean isPkCol = columnFamily == null;
-byte[] columnKey = getColumnKey(viewKey, columnName,
columnFamily);
+
+boolean isColumnToBeAddPkCol = columnFamily == null;
if (existingViewColumn != null) {
-MetaDataMutationResult result =
validateColumnForAddToBaseTable(existingViewColumn, baseTableColumnPut,
basePhysicalTable, isPkCol, view);
-if (result != null) {
-return result;
+if
(EncodedColumnsUtil.usesEncodedColumnNames(basePhysicalTable)
+&& !SchemaUtil.isPKColumn(existingViewColumn))
{
--- End diff --
Is there a race condition with this check and would the be covered by one
of the future JIRAs you mentioned?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200205809
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -586,48 +590,359 @@ public void getTable(RpcController controller,
GetTableRequest request,
builder.setMutationTime(minNonZerodisableIndexTimestamp - 1);
}
}
-
-if (table.getTimeStamp() != tableTimeStamp) {
+// the PTable of views and indexes on views might get updated
because a column is added to one of
+// their parents (this won't change the timestamp)
+if (table.getType()!=PTableType.TABLE || table.getTimeStamp()
!= tableTimeStamp) {
builder.setTable(PTableImpl.toProto(table));
}
done.run(builder.build());
-return;
} catch (Throwable t) {
logger.error("getTable failed", t);
ProtobufUtil.setControllerException(controller,
ServerUtil.createIOException(SchemaUtil.getTableName(schemaName, tableName),
t));
}
}
+/**
+ * Used to add the columns present the ancestor hierarchy to the
PTable of the given view or
+ * view index
+ * @param table PTable of the view or view index
+ * @param skipAddingIndexes if true the returned PTable won't include
indexes
+ * @param skipAddingParentColumns if true the returned PTable won't
include columns derived from
+ *ancestor tables
+ * @param lockedAncestorTable ancestor table table that is being
mutated (as we won't be able to
+ *resolve this table as its locked)
+ */
+private Pair
combineColumns(PTable table, long timestamp,
+int clientVersion, boolean skipAddingIndexes, boolean
skipAddingParentColumns,
+PTable lockedAncestorTable) throws SQLException, IOException {
+boolean hasIndexId = table.getViewIndexId() != null;
+if (table.getType() != PTableType.VIEW && !hasIndexId) {
+return new Pair(table,
+MetaDataProtos.MutationCode.TABLE_ALREADY_EXISTS);
+}
+if (!skipAddingParentColumns) {
+table =
+addDerivedColumnsFromAncestors(table, timestamp,
clientVersion,
+lockedAncestorTable);
+if (table==null) {
+return new Pair(table,
+MetaDataProtos.MutationCode.TABLE_NOT_FOUND);
+}
+// we need to resolve the indexes of views (to get ensure they
also have all the columns
+// derived from their ancestors)
+if (!skipAddingIndexes && !table.getIndexes().isEmpty()) {
+List indexes =
Lists.newArrayListWithExpectedSize(table.getIndexes().size());
+for (PTable index : table.getIndexes()) {
+byte[] tenantIdBytes =
+index.getTenantId() == null ?
ByteUtil.EMPTY_BYTE_ARRAY
+: index.getTenantId().getBytes();
+PTable latestIndex =
+doGetTable(tenantIdBytes,
index.getSchemaName().getBytes(),
+index.getTableName().getBytes(),
timestamp, null, clientVersion, true,
+false, lockedAncestorTable);
+if (latestIndex == null) {
+throw new TableNotFoundException(
+"Could not find index table while
combining columns "
++ index.getTableName().getString()
+ " with tenant id "
++ index.getTenantId());
+}
+indexes.add(latestIndex);
+}
+table = PTableImpl.makePTable(table, table.getTimeStamp(),
indexes);
+}
+}
+
+MetaDataProtos.MutationCode mutationCode =
+table != null ?
MetaDataProtos.MutationCode.TABLE_ALREADY_EXISTS
+: MetaDataProtos.MutationCode.TABLE_NOT_FOUND;
+return new Pair(table,
mutationCode);
+}
+
+
+private PTable addDerivedColumnsFromAncestors(PTable table, long
timestamp,
+int clientVersion, PTable lockedAncestorTable) throws
IOException, SQLException, TableNotFoundException {
+// combine columns for view and view indexes
+byte[] tenantId =
+
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206109
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -586,48 +590,359 @@ public void getTable(RpcController controller,
GetTableRequest request,
builder.setMutationTime(minNonZerodisableIndexTimestamp - 1);
}
}
-
-if (table.getTimeStamp() != tableTimeStamp) {
+// the PTable of views and indexes on views might get updated
because a column is added to one of
+// their parents (this won't change the timestamp)
+if (table.getType()!=PTableType.TABLE || table.getTimeStamp()
!= tableTimeStamp) {
builder.setTable(PTableImpl.toProto(table));
}
done.run(builder.build());
-return;
} catch (Throwable t) {
logger.error("getTable failed", t);
ProtobufUtil.setControllerException(controller,
ServerUtil.createIOException(SchemaUtil.getTableName(schemaName, tableName),
t));
}
}
+/**
+ * Used to add the columns present the ancestor hierarchy to the
PTable of the given view or
+ * view index
+ * @param table PTable of the view or view index
+ * @param skipAddingIndexes if true the returned PTable won't include
indexes
+ * @param skipAddingParentColumns if true the returned PTable won't
include columns derived from
+ *ancestor tables
+ * @param lockedAncestorTable ancestor table table that is being
mutated (as we won't be able to
+ *resolve this table as its locked)
+ */
+private Pair
combineColumns(PTable table, long timestamp,
+int clientVersion, boolean skipAddingIndexes, boolean
skipAddingParentColumns,
+PTable lockedAncestorTable) throws SQLException, IOException {
+boolean hasIndexId = table.getViewIndexId() != null;
+if (table.getType() != PTableType.VIEW && !hasIndexId) {
+return new Pair(table,
+MetaDataProtos.MutationCode.TABLE_ALREADY_EXISTS);
+}
+if (!skipAddingParentColumns) {
+table =
+addDerivedColumnsFromAncestors(table, timestamp,
clientVersion,
+lockedAncestorTable);
+if (table==null) {
+return new Pair(table,
+MetaDataProtos.MutationCode.TABLE_NOT_FOUND);
+}
+// we need to resolve the indexes of views (to get ensure they
also have all the columns
+// derived from their ancestors)
+if (!skipAddingIndexes && !table.getIndexes().isEmpty()) {
+List indexes =
Lists.newArrayListWithExpectedSize(table.getIndexes().size());
+for (PTable index : table.getIndexes()) {
+byte[] tenantIdBytes =
+index.getTenantId() == null ?
ByteUtil.EMPTY_BYTE_ARRAY
+: index.getTenantId().getBytes();
+PTable latestIndex =
+doGetTable(tenantIdBytes,
index.getSchemaName().getBytes(),
+index.getTableName().getBytes(),
timestamp, null, clientVersion, true,
+false, lockedAncestorTable);
+if (latestIndex == null) {
+throw new TableNotFoundException(
+"Could not find index table while
combining columns "
++ index.getTableName().getString()
+ " with tenant id "
++ index.getTenantId());
+}
+indexes.add(latestIndex);
+}
+table = PTableImpl.makePTable(table, table.getTimeStamp(),
indexes);
+}
+}
+
+MetaDataProtos.MutationCode mutationCode =
+table != null ?
MetaDataProtos.MutationCode.TABLE_ALREADY_EXISTS
+: MetaDataProtos.MutationCode.TABLE_NOT_FOUND;
+return new Pair(table,
mutationCode);
+}
+
+
+private PTable addDerivedColumnsFromAncestors(PTable table, long
timestamp,
+int clientVersion, PTable lockedAncestorTable) throws
IOException, SQLException, TableNotFoundException {
+// combine columns for view and view indexes
+byte[] tenantId =
+
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200207100
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -2147,46 +2566,29 @@ private MetaDataMutationResult doDropTable(byte[]
key, byte[] tenantId, byte[] s
}
if (tableType == PTableType.TABLE || tableType ==
PTableType.SYSTEM) {
-// Handle any child views that exist
-TableViewFinder tableViewFinderResult =
findChildViews(region, tenantId, table, clientVersion, !isCascade);
-if (tableViewFinderResult.hasViews()) {
-if (isCascade) {
-if
(tableViewFinderResult.allViewsInMultipleRegions()) {
-// We don't yet support deleting a table with
views where SYSTEM.CATALOG has split and the
-// view metadata spans multiple regions
-return new
MetaDataMutationResult(MutationCode.UNALLOWED_TABLE_MUTATION,
-
EnvironmentEdgeManager.currentTimeMillis(), null);
-} else if
(tableViewFinderResult.allViewsInSingleRegion()) {
-// Recursively delete views - safe as all the
views as all in the same region
-for (ViewInfo viewInfo :
tableViewFinderResult.getViewInfoList()) {
-byte[] viewTenantId =
viewInfo.getTenantId();
-byte[] viewSchemaName =
viewInfo.getSchemaName();
-byte[] viewName = viewInfo.getViewName();
-byte[] viewKey =
SchemaUtil.getTableKey(viewTenantId, viewSchemaName, viewName);
-Delete delete = new Delete(viewKey,
clientTimeStamp);
-rowsToDelete.add(delete);
-acquireLock(region, viewKey, locks);
-MetaDataMutationResult result =
doDropTable(viewKey, viewTenantId, viewSchemaName,
-viewName, null, PTableType.VIEW,
rowsToDelete, invalidateList, locks,
-tableNamesToDelete,
sharedTablesToDelete, false, clientVersion);
-if (result.getMutationCode() !=
MutationCode.TABLE_ALREADY_EXISTS) { return result; }
-}
-}
-} else {
+// check to see if the table has any child views
+try (Table hTable =
+env.getTable(SchemaUtil.getPhysicalTableName(
+
PhoenixDatabaseMetaData.SYSTEM_CHILD_LINK_NAME_BYTES,
+env.getConfiguration( {
+boolean hasChildViews =
+ViewFinder.hasChildViews(hTable, tenantId,
schemaName, tableName,
+clientTimeStamp);
+if (hasChildViews && !isCascade) {
--- End diff --
Isn't there a race condition with this check?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206293
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1457,28 +1791,110 @@ private static void getSchemaTableNames(Mutation
row, byte[][] schemaTableNames)
schemaTableNames[2] = tName;
}
}
-
+
@Override
public void createTable(RpcController controller, CreateTableRequest
request,
RpcCallback done) {
MetaDataResponse.Builder builder = MetaDataResponse.newBuilder();
byte[][] rowKeyMetaData = new byte[3][];
byte[] schemaName = null;
byte[] tableName = null;
+String fullTableName = null;
try {
int clientVersion = request.getClientVersion();
List tableMetadata =
ProtobufUtil.getMutations(request);
MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata,
rowKeyMetaData);
byte[] tenantIdBytes =
rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX];
schemaName =
rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
tableName =
rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
+fullTableName = SchemaUtil.getTableName(schemaName, tableName);
+// TODO before creating a table we need to see if the table
was previously created and then dropped
+// and clean up any parent->child links or child views
--- End diff --
Remove TODO as isn't this done now?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206428
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1488,20 +1904,19 @@ public void createTable(RpcController controller,
CreateTableRequest request,
if (tableType == PTableType.VIEW) {
byte[][] parentSchemaTableNames = new byte[3][];
byte[][] parentPhysicalSchemaTableNames = new byte[3][];
-/*
- * For a view, we lock the base physical table row. For a
mapped view, there is
- * no link present to the physical table. So the
viewPhysicalTableRow is null
- * in that case.
- */
+ /*
+* For a mapped view, there is no link present
to the physical table. So the
+* viewPhysicalTableRow is null in that case.
+*/
--- End diff --
Fix indentation
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206792
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1809,180 +2256,97 @@ public void createTable(RpcController controller,
CreateTableRequest request,
} catch (Throwable t) {
logger.error("createTable failed", t);
ProtobufUtil.setControllerException(controller,
-
ServerUtil.createIOException(SchemaUtil.getTableName(schemaName, tableName),
t));
+ServerUtil.createIOException(fullTableName, t));
}
}
+ private void dropChildMetadata(byte[] schemaName, byte[] tableName,
byte[] tenantIdBytes)
+ throws IOException, SQLException,
ClassNotFoundException {
+ TableViewFinderResult childViewsResult = new
TableViewFinderResult();
+ findAllChildViews(tenantIdBytes, schemaName, tableName,
childViewsResult);
+ if (childViewsResult.hasViews()) {
+ for (TableInfo viewInfo :
childViewsResult.getResults()) {
+ byte[] viewTenantId = viewInfo.getTenantId();
+ byte[] viewSchemaName =
viewInfo.getSchemaName();
+ byte[] viewName = viewInfo.getTableName();
+ Properties props = new Properties();
+ if (viewTenantId != null && viewTenantId.length
!= 0)
+
props.setProperty(PhoenixRuntime.TENANT_ID_ATTRIB,
Bytes.toString(viewTenantId));
+ try (PhoenixConnection connection =
QueryUtil.getConnectionOnServer(env.getConfiguration())
+
.unwrap(PhoenixConnection.class)) {
+ MetaDataClient client = new
MetaDataClient(connection);
+ org.apache.phoenix.parse.TableName
viewTableName = org.apache.phoenix.parse.TableName
+
.create(Bytes.toString(viewSchemaName), Bytes.toString(viewName));
+ client.dropTable(
+ new
DropTableStatement(viewTableName, PTableType.VIEW, false, true, true));
+ }
+ }
+ }
+ }
+
private boolean execeededIndexQuota(PTableType tableType, PTable
parentTable) {
return PTableType.INDEX == tableType &&
parentTable.getIndexes().size() >= maxIndexesPerTable;
}
-
-private void findAllChildViews(Region region, byte[] tenantId, PTable
table,
-TableViewFinder result, long clientTimeStamp, int
clientVersion) throws IOException, SQLException {
-TableViewFinder currResult = findChildViews(region, tenantId,
table, clientVersion, false);
-result.addResult(currResult);
-for (ViewInfo viewInfo : currResult.getViewInfoList()) {
-byte[] viewtenantId = viewInfo.getTenantId();
-byte[] viewSchema = viewInfo.getSchemaName();
-byte[] viewTable = viewInfo.getViewName();
-byte[] tableKey = SchemaUtil.getTableKey(viewtenantId,
viewSchema, viewTable);
-ImmutableBytesPtr cacheKey = new ImmutableBytesPtr(tableKey);
-PTable view = loadTable(env, tableKey, cacheKey,
clientTimeStamp, clientTimeStamp, clientVersion);
-if (view == null) {
-logger.warn("Found orphan tenant view row in
SYSTEM.CATALOG with tenantId:"
-+ Bytes.toString(tenantId) + ", schema:"
-+ Bytes.toString(viewSchema) + ", table:"
-+ Bytes.toString(viewTable));
-continue;
-}
-findAllChildViews(region, viewtenantId, view, result,
clientTimeStamp, clientVersion);
-}
-}
-
-// TODO use child link instead once splittable system catalog
(PHOENIX-3534) is implemented
-// and we have a separate table for links.
-private TableViewFinder findChildViews_deprecated(Region region,
byte[] tenantId, PTable table, byte[] linkTypeBytes, boolean stopAfterFirst)
throws IOException {
-byte[] schemaName = table.getSchemaName().getBytes();
-byte[] tableName = table.getTableName().getBytes();
-boolean isMultiTenant = table.isMultiTenant();
-Scan scan = new Scan();
-// If the table is multi-tenant, we need to check across all
tenant_ids,
-// so we can't constrain the row key. Ot
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r200206389
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1457,28 +1791,110 @@ private static void getSchemaTableNames(Mutation
row, byte[][] schemaTableNames)
schemaTableNames[2] = tName;
}
}
-
+
@Override
public void createTable(RpcController controller, CreateTableRequest
request,
RpcCallback done) {
MetaDataResponse.Builder builder = MetaDataResponse.newBuilder();
byte[][] rowKeyMetaData = new byte[3][];
byte[] schemaName = null;
byte[] tableName = null;
+String fullTableName = null;
try {
int clientVersion = request.getClientVersion();
List tableMetadata =
ProtobufUtil.getMutations(request);
MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata,
rowKeyMetaData);
byte[] tenantIdBytes =
rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX];
schemaName =
rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
tableName =
rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
+fullTableName = SchemaUtil.getTableName(schemaName, tableName);
+// TODO before creating a table we need to see if the table
was previously created and then dropped
+// and clean up any parent->child links or child views
boolean isNamespaceMapped =
MetaDataUtil.isNameSpaceMapped(tableMetadata, GenericKeyValueBuilder.INSTANCE,
new ImmutableBytesWritable());
final IndexType indexType =
MetaDataUtil.getIndexType(tableMetadata, GenericKeyValueBuilder.INSTANCE,
new ImmutableBytesWritable());
+byte[] parentTenantId = null;
byte[] parentSchemaName = null;
byte[] parentTableName = null;
PTableType tableType =
MetaDataUtil.getTableType(tableMetadata, GenericKeyValueBuilder.INSTANCE, new
ImmutableBytesWritable());
+ViewType viewType = MetaDataUtil.getViewType(tableMetadata,
GenericKeyValueBuilder.INSTANCE, new ImmutableBytesWritable());
+
+// Load table to see if it already exists
+byte[] tableKey = SchemaUtil.getTableKey(tenantIdBytes,
schemaName, tableName);
+ImmutableBytesPtr cacheKey = new ImmutableBytesPtr(tableKey);
+long clientTimeStamp =
MetaDataUtil.getClientTimeStamp(tableMetadata);
+PTable table = null;
+ try {
+ // Get as of latest timestamp so we can detect
if we have a newer table that already
+ // exists without making an additional query
+ table = loadTable(env, tableKey, cacheKey,
clientTimeStamp, HConstants.LATEST_TIMESTAMP,
+ clientVersion);
+ } catch (ParentTableNotFoundException e) {
+ dropChildMetadata(e.getParentSchemaName(),
e.getParentTableName(), e.getParentTenantId());
+ }
+if (table != null) {
+if (table.getTimeStamp() < clientTimeStamp) {
+// If the table is older than the client time stamp
and it's deleted,
+// continue
+if (!isTableDeleted(table)) {
+
builder.setReturnCode(MetaDataProtos.MutationCode.TABLE_ALREADY_EXISTS);
+
builder.setMutationTime(EnvironmentEdgeManager.currentTimeMillis());
+builder.setTable(PTableImpl.toProto(table));
+done.run(builder.build());
+return;
+}
+} else {
+
builder.setReturnCode(MetaDataProtos.MutationCode.NEWER_TABLE_FOUND);
+
builder.setMutationTime(EnvironmentEdgeManager.currentTimeMillis());
+builder.setTable(PTableImpl.toProto(table));
+done.run(builder.build());
+return;
+}
+}
+
+ // check if the table was dropped, but had child views
that were have not yet
+ // been cleaned up by compaction
+ if
(!Bytes.toString(schemaName).equals(QueryConstants.SYSTEM_SCHEMA_NAME)) {
+ dropChildMetadata(schemaName, tableName,
tenantIdBytes);
+ }
--- End diff --
Minor - indentation issue here.
---
[GitHub] phoenix issue #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG table
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/303 +1 to the patch. Great work @twdsilva and @churrodog! I made some minor comments for some potential follow up work and had a few questions, but let's get this committed first. I'd recommend the following priority for the next JIRA as: 1. Move views to their own table 2. Get rid of client side code that is sending the base columns 3. Fix corner case/race condition issues 4. Add code that doesn't write orphaned metadata on major compaction ---
[GitHub] phoenix pull request #305: Omid2
GitHub user JamesRTaylor opened a pull request: https://github.com/apache/phoenix/pull/305 Omid2 You can merge this pull request into a Git repository by running: $ git pull https://github.com/apache/phoenix omid2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/305.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 #305 commit 1039f9d4ca0da5185d8babaa6681459e07da61ad Author: Ohad Shacham Date: 2018-04-03T14:03:55Z Add Omid support for Phoenix commit a11bddcacb90d553f6c0fdab37e2c335f7f23ac4 Author: Ohad Shacham Date: 2018-05-02T11:55:47Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 45e6b0e309d037c03221de249501fa7c4bc651db Author: Ohad Shacham Date: 2018-05-08T07:16:19Z Remove hard coded Omid commit 18bea3907ef73c6f842ac5410ea4623be5a36b18 Author: Ohad Shacham Date: 2018-05-08T09:28:05Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 38ab6f459e6174e150d88a146b4a35d9c1857fb6 Author: Ohad Shacham Date: 2018-05-15T08:19:05Z some merge fixes commit cbab9b72ee5c62d9512b9472b010f581e3866e21 Author: Ohad Shacham Date: 2018-05-22T13:04:56Z Fix hbase config commit 7c834994a862f5e2a0edfa560c0a1c4f047383ca Author: Ohad Shacham Date: 2018-05-22T18:30:01Z Partially revert the following commits. https://github.com/ohadshacham/phoenix/commit/45e6b0e309d037c03221de249501fa7c4bc651db https://github.com/ohadshacham/phoenix/commit/38ab6f459e6174e150d88a146b4a35d9c1857fb6 commit dc6de3ab12ad18e9dc5622521bfaf5a662e4d409 Author: Ohad Shacham Date: 2018-05-22T18:32:35Z Merge commit '2015345a023f0adb59174443ec1328bb1399f11b' into 4.x-HBase-1.3-Omid-2 commit 9eae4abdbc770381620e23f4793acb279cec2544 Author: Ohad Shacham Date: 2018-05-22T18:42:26Z Change back to TEPHRA what needed for testing TEPHRA commit 0c5dfd642b3738d2d09aa19f2e6d4cbb97852c20 Author: Ohad Shacham Date: 2018-05-23T12:14:37Z remove unnecessary changes. commit 8d8d0e37210460aa00d387956eb013b96bd5de38 Author: Ohad Shacham Date: 2018-05-24T12:58:14Z Remove dependency in testng commit b1fcc46c277493b046f92358d0e9403b65815e23 Author: James Taylor Date: 2018-05-26T14:40:33Z Include transaction provider code in transaction bytes send to server and fix misc tests commit 3a9cd245349d05a0ab0a97dbfd26921a623567cb Author: Ohad Shacham Date: 2018-05-29T12:54:00Z Fix population of local index and move markPutAsCommitted to PhoenixTransactionContext. commit 9a378cdc49b76d839ca1e1549a7a657dd7c19a45 Author: James Taylor Date: 2018-05-30T16:20:40Z Perform local index maintainence on client side for Omid commit 77586b857cec36c63a75bd87e9e83a375bc68bb7 Author: James Taylor Date: 2018-05-30T19:47:15Z Fix Tephra regressions from previous commit commit f563678cf174805bbb68b60bcf235a582cf2b2c3 Author: James Taylor Date: 2018-05-31T04:51:29Z Fix for PartialCommitIT with Omid commit a220bc5eaa173604c01373759938e8b2867082e8 Author: James Taylor Date: 2018-05-31T16:09:47Z Remove unneeded PhoenixTransactionalTable commit 57a1f11ae4aa02633e95436e7b32945d3803ac65 Author: James Taylor Date: 2018-05-31T17:13:24Z Remove statics in OmidTransactionProcessor (but still could use more work) commit 1e35aa2b0a3df3e5b3ebc9b6fac186da5ac0d1b8 Author: James Taylor Date: 2018-06-01T20:15:43Z Fix asynchronous index building for Omid commit 629e4873c212379dfab2dd12d80681ca9c2330ed Author: James Taylor Date: 2018-06-03T06:00:05Z Fix initial population of local indexes for Omid commit 5a1d161566ed5a1532ed24978a48ee9c32c8ece4 Author: James Taylor Date: 2018-06-03T07:09:46Z Fix client-side local index deletes commit d1977fff3d78389f479963c71955b2e33e3c26c6 Author: James Taylor Date: 2018-06-03T16:33:43Z Comment failing test with workaround commit ec7a66072d616f740bfd669b8da56d0ac995f4a1 Author: James Taylor Date: 2018-06-12T06:08:31Z Catch up omid2 branch with latest from 4.x-HBase-1.3 commit 64bd216c3534bc99f0803f3e442878e2bc650474 Author: James Taylor Date: 2018-06-14T17:01:26Z Fixes and parameterization of unit tests commit 119d5b48f5aef79e5edeade958481f8edbfdeec9 Author: James Taylor Date: 2018-06-14T17:47:55Z Fix compilation errors commit f925191568cfcae2373d1152aeaebfd61fa96c39 Author: James Taylor Date: 2018-06-14T18:04:21Z Parameterize BaseViewIT and BaseIndexIT commit a7cd47da6a8fb849358f0fd5b7b0a324d66584d2 Author: James Taylor Date: 2018-06-14T20:09:30Z Parameterize transactional tests for both Tephra and Omid commit aa85ed4605d242b9e3db895e2cb2cf92553ab480 Author: James Taylor Date: 2018-06-15T14:21:51Z Parameterize transactional
[GitHub] phoenix issue #298: PHOENIX-4666 Persistent subquery cache for hash joins
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/298 Is this ready to go, @ortutay? It looks pretty clean now. ---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r195421295
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
---
@@ -1305,14 +1305,25 @@ public ScanWrapper(Scan scan) {
Scan oldScan = scanPair.getFirst();
byte[] startKey =
oldScan.getAttribute(SCAN_ACTUAL_START_ROW);
if(e2 instanceof
HashJoinCacheNotFoundException){
+System.out.println("Handling
HashJoinCacheNotFoundException");
--- End diff --
Minor nit: change to log message or remove System.out.println calls
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r192570609
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java
---
@@ -388,51 +435,65 @@ public void
testViewAndTableInDifferentSchemas(boolean isNamespaceMapped) throws
} catch (TableNotFoundException ignore) {
}
ddl = "DROP TABLE " + fullTableName1;
-validateCannotDropTableWithChildViewsWithoutCascade(conn,
fullTableName1);
ddl = "DROP VIEW " + fullViewName2;
conn.createStatement().execute(ddl);
ddl = "DROP TABLE " + fullTableName1;
conn.createStatement().execute(ddl);
}
-
+
@Test
-public void testDisallowDropOfColumnOnParentTable() throws Exception {
+public void testDropOfColumnOnParentTableInvalidatesView() throws
Exception {
Connection conn = DriverManager.getConnection(getUrl());
+String fullTableName = generateUniqueTableName();
+String viewName = generateUniqueViewName();
+splitSystemCatalog(Lists.newArrayList(fullTableName, viewName));
+
String ddl = "CREATE TABLE " + fullTableName + " (k1 INTEGER NOT
NULL, k2 INTEGER NOT NULL, v1 DECIMAL, CONSTRAINT pk PRIMARY KEY (k1, k2))" +
tableDDLOptions;
conn.createStatement().execute(ddl);
-String viewName = "V_" + generateUniqueName();
ddl = "CREATE VIEW " + viewName + "(v2 VARCHAR, v3 VARCHAR) AS
SELECT * FROM " + fullTableName + " WHERE v1 = 1.0";
conn.createStatement().execute(ddl);
-try {
-conn.createStatement().execute("ALTER TABLE " + fullTableName
+ " DROP COLUMN v1");
-fail();
-} catch (SQLException e) {
-
assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(),
e.getErrorCode());
+conn.createStatement().execute("ALTER TABLE " + fullTableName + "
DROP COLUMN v1");
+// TODO see if its possibel to prevent the dropping of a column
thats required by a child view (for its view where clause)
+// the view should be invalid
--- End diff --
Ok, this sounds like it'll work fine.
---
[GitHub] phoenix issue #300: Omid transaction support in Phoenix
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/300 Opened on behalf of @ohadshacham . ---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r192309043
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java
---
@@ -388,51 +435,65 @@ public void
testViewAndTableInDifferentSchemas(boolean isNamespaceMapped) throws
} catch (TableNotFoundException ignore) {
}
ddl = "DROP TABLE " + fullTableName1;
-validateCannotDropTableWithChildViewsWithoutCascade(conn,
fullTableName1);
ddl = "DROP VIEW " + fullViewName2;
conn.createStatement().execute(ddl);
ddl = "DROP TABLE " + fullTableName1;
conn.createStatement().execute(ddl);
}
-
+
@Test
-public void testDisallowDropOfColumnOnParentTable() throws Exception {
+public void testDropOfColumnOnParentTableInvalidatesView() throws
Exception {
Connection conn = DriverManager.getConnection(getUrl());
+String fullTableName = generateUniqueTableName();
+String viewName = generateUniqueViewName();
+splitSystemCatalog(Lists.newArrayList(fullTableName, viewName));
+
String ddl = "CREATE TABLE " + fullTableName + " (k1 INTEGER NOT
NULL, k2 INTEGER NOT NULL, v1 DECIMAL, CONSTRAINT pk PRIMARY KEY (k1, k2))" +
tableDDLOptions;
conn.createStatement().execute(ddl);
-String viewName = "V_" + generateUniqueName();
ddl = "CREATE VIEW " + viewName + "(v2 VARCHAR, v3 VARCHAR) AS
SELECT * FROM " + fullTableName + " WHERE v1 = 1.0";
conn.createStatement().execute(ddl);
-try {
-conn.createStatement().execute("ALTER TABLE " + fullTableName
+ " DROP COLUMN v1");
-fail();
-} catch (SQLException e) {
-
assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(),
e.getErrorCode());
+conn.createStatement().execute("ALTER TABLE " + fullTableName + "
DROP COLUMN v1");
+// TODO see if its possibel to prevent the dropping of a column
thats required by a child view (for its view where clause)
+// the view should be invalid
--- End diff --
I think we should avoid needing any kind of locking (including a
checkAndPut). The scaling issues we ran into were caused by contention on a
lock for the parent. The same thing could happen again with a checkAndPut
(which is just locking the row during the check). I'd err on the side of
scalability and have a view be invalidated if its parent is deleted. I think
that's a more scalable solution.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r192308674
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/coprocessor/MetaDataEndpointImplTest.java
---
@@ -0,0 +1,299 @@
+package org.apache.phoenix.coprocessor;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.HTable;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData;
+import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.TableNotFoundException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+
+/**
+ * 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.
+ */
+public class MetaDataEndpointImplTest extends ParallelStatsDisabledIT {
--- End diff --
I think we should keep the OrphanCleaner code. The idea is that failed
deletions of metadata are transparent. They shouldn't block creation of new
tables. We should also cleanup on compaction.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191944884
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java
---
@@ -388,51 +435,65 @@ public void
testViewAndTableInDifferentSchemas(boolean isNamespaceMapped) throws
} catch (TableNotFoundException ignore) {
}
ddl = "DROP TABLE " + fullTableName1;
-validateCannotDropTableWithChildViewsWithoutCascade(conn,
fullTableName1);
ddl = "DROP VIEW " + fullViewName2;
conn.createStatement().execute(ddl);
ddl = "DROP TABLE " + fullTableName1;
conn.createStatement().execute(ddl);
}
-
+
@Test
-public void testDisallowDropOfColumnOnParentTable() throws Exception {
+public void testDropOfColumnOnParentTableInvalidatesView() throws
Exception {
Connection conn = DriverManager.getConnection(getUrl());
+String fullTableName = generateUniqueTableName();
+String viewName = generateUniqueViewName();
+splitSystemCatalog(Lists.newArrayList(fullTableName, viewName));
+
String ddl = "CREATE TABLE " + fullTableName + " (k1 INTEGER NOT
NULL, k2 INTEGER NOT NULL, v1 DECIMAL, CONSTRAINT pk PRIMARY KEY (k1, k2))" +
tableDDLOptions;
conn.createStatement().execute(ddl);
-String viewName = "V_" + generateUniqueName();
ddl = "CREATE VIEW " + viewName + "(v2 VARCHAR, v3 VARCHAR) AS
SELECT * FROM " + fullTableName + " WHERE v1 = 1.0";
conn.createStatement().execute(ddl);
-try {
-conn.createStatement().execute("ALTER TABLE " + fullTableName
+ " DROP COLUMN v1");
-fail();
-} catch (SQLException e) {
-
assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(),
e.getErrorCode());
+conn.createStatement().execute("ALTER TABLE " + fullTableName + "
DROP COLUMN v1");
+// TODO see if its possibel to prevent the dropping of a column
thats required by a child view (for its view where clause)
+// the view should be invalid
--- End diff --
Seems like there'd be an inherent race condition, but if not, I suppose
keeping the same behavior is fine. What about preventing the drop of a table
with child views?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191942250
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -2227,35 +2551,36 @@ private MetaDataMutationResult doDropTable(byte[]
key, byte[] tenantId, byte[] s
// in 0.94.4, thus if we try to use it here we can no longer
use the 0.94.2 version
// of the client.
Delete delete = new Delete(indexKey, clientTimeStamp);
-rowsToDelete.add(delete);
-acquireLock(region, indexKey, locks);
+catalogMutations.add(delete);
MetaDataMutationResult result =
doDropTable(indexKey, tenantId, schemaName, indexName,
tableName, PTableType.INDEX,
-rowsToDelete, invalidateList, locks,
tableNamesToDelete, sharedTablesToDelete, false, clientVersion);
+catalogMutations, childLinkMutations,
invalidateList, tableNamesToDelete, sharedTablesToDelete, false, clientVersion);
if (result.getMutationCode() !=
MutationCode.TABLE_ALREADY_EXISTS) {
return result;
}
}
+// no need to pass sharedTablesToDelete back to the client as they
deletion of these tables
+// is already handled in MetadataClient.dropTable
--- End diff --
Not sure if this is handled differently now, but we passed this back
because I believe we don't know on the client all of the physical index tables
to delete. I think we have a test for this.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191940463
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1995,36 +2287,46 @@ public void dropTable(RpcController controller,
DropTableRequest request,
try {
List tableMetadata =
ProtobufUtil.getMutations(request);
+List childLinkMutations = Lists.newArrayList();
MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata,
rowKeyMetaData);
byte[] tenantIdBytes =
rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX];
schemaName =
rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
tableName =
rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
+PTableType
pTableType=PTableType.fromSerializedValue(tableType);
// Disallow deletion of a system table
-if (tableType.equals(PTableType.SYSTEM.getSerializedValue())) {
+if (pTableType == PTableType.SYSTEM) {
builder.setReturnCode(MetaDataProtos.MutationCode.UNALLOWED_TABLE_MUTATION);
builder.setMutationTime(EnvironmentEdgeManager.currentTimeMillis());
done.run(builder.build());
return;
}
+
List tableNamesToDelete = Lists.newArrayList();
List sharedTablesToDelete =
Lists.newArrayList();
-// No need to lock parent table for views
-byte[] parentTableName =
MetaDataUtil.getParentTableName(tableMetadata);
-byte[] lockTableName = parentTableName == null ||
tableType.equals(PTableType.VIEW.getSerializedValue()) ? tableName :
parentTableName;
-byte[] lockKey = SchemaUtil.getTableKey(tenantIdBytes,
schemaName, lockTableName);
-byte[] key =
-parentTableName == null ? lockKey :
SchemaUtil.getTableKey(tenantIdBytes,
-schemaName, tableName);
+
+byte[] lockKey = SchemaUtil.getTableKey(tenantIdBytes,
schemaName, tableName);
Region region = env.getRegion();
-MetaDataMutationResult result = checkTableKeyInRegion(key,
region);
+MetaDataMutationResult result = checkTableKeyInRegion(lockKey,
region);
if (result != null) {
done.run(MetaDataMutationResult.toProto(result));
return;
}
-PTableType
ptableType=PTableType.fromSerializedValue(tableType);
+
+byte[] parentTableName =
MetaDataUtil.getParentTableName(tableMetadata);
+byte[] parentLockKey = null;
+// No need to lock parent table for views
+if (parentTableName != null && pTableType != PTableType.VIEW) {
+parentLockKey = SchemaUtil.getTableKey(tenantIdBytes,
schemaName, parentTableName);
--- End diff --
Shouldn't need this parentLockKey any longer, yes? Or is this only for
indexes?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191939788
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -1457,28 +1761,69 @@ private static void getSchemaTableNames(Mutation
row, byte[][] schemaTableNames)
schemaTableNames[2] = tName;
}
}
-
+
@Override
public void createTable(RpcController controller, CreateTableRequest
request,
RpcCallback done) {
MetaDataResponse.Builder builder = MetaDataResponse.newBuilder();
byte[][] rowKeyMetaData = new byte[3][];
byte[] schemaName = null;
byte[] tableName = null;
+String fullTableName = SchemaUtil.getTableName(schemaName,
tableName);
try {
int clientVersion = request.getClientVersion();
List tableMetadata =
ProtobufUtil.getMutations(request);
MetaDataUtil.getTenantIdAndSchemaAndTableName(tableMetadata,
rowKeyMetaData);
byte[] tenantIdBytes =
rowKeyMetaData[PhoenixDatabaseMetaData.TENANT_ID_INDEX];
schemaName =
rowKeyMetaData[PhoenixDatabaseMetaData.SCHEMA_NAME_INDEX];
tableName =
rowKeyMetaData[PhoenixDatabaseMetaData.TABLE_NAME_INDEX];
+// TODO before creating a table we need to see if the table
was previously created and then dropped
+// and clean up any parent->child links or child views
boolean isNamespaceMapped =
MetaDataUtil.isNameSpaceMapped(tableMetadata, GenericKeyValueBuilder.INSTANCE,
new ImmutableBytesWritable());
final IndexType indexType =
MetaDataUtil.getIndexType(tableMetadata, GenericKeyValueBuilder.INSTANCE,
new ImmutableBytesWritable());
+byte[] parentTenantId = null;
byte[] parentSchemaName = null;
byte[] parentTableName = null;
PTableType tableType =
MetaDataUtil.getTableType(tableMetadata, GenericKeyValueBuilder.INSTANCE, new
ImmutableBytesWritable());
+ViewType viewType = MetaDataUtil.getViewType(tableMetadata,
GenericKeyValueBuilder.INSTANCE, new ImmutableBytesWritable());
+
+// Here we are passed the parent's columns to add to a view,
PHOENIX-3534 allows for a splittable
+// System.Catalog thus we only store the columns that are new
to the view, not the parents columns,
+// thus here we remove everything that is ORDINAL.POSITION <=
baseColumnCount and update the
+// ORDINAL.POSITIONS to be shifted accordingly.
--- End diff --
Important to file and reference a JIRA here to remove the dedup code once
clients have been upgraded to the release in which we no longer send the
duplicate information. Can we stop sending the duplicate info in the same
release that SYSTEM.CATALOG becomes splittable? Seems like yes.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191938147
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
---
@@ -586,48 +573,336 @@ public void getTable(RpcController controller,
GetTableRequest request,
builder.setMutationTime(minNonZerodisableIndexTimestamp - 1);
}
}
-
-if (table.getTimeStamp() != tableTimeStamp) {
+// the PTable of views and indexes on views might get updated
because a column is added to one of
+// their parents (this won't change the timestamp)
+if (table.getType()!=PTableType.TABLE || table.getTimeStamp()
!= tableTimeStamp) {
builder.setTable(PTableImpl.toProto(table));
}
done.run(builder.build());
-return;
} catch (Throwable t) {
logger.error("getTable failed", t);
ProtobufUtil.setControllerException(controller,
ServerUtil.createIOException(SchemaUtil.getTableName(schemaName, tableName),
t));
}
}
+/**
+ * Used to add the columns present the ancestor hierarchy to the
PTable of the given view or
+ * view index
+ * @param table PTable of the view or view index
+ * @param skipAddingIndexes if true the returned PTable won't include
indexes
+ * @param skipAddingParentColumns if true the returned PTable won't
include columns derived from ancestor tables
+ */
+private Pair
combineColumns(PTable table, long timestamp,
+int clientVersion, boolean skipAddingIndexes, boolean
skipAddingParentColumns) throws SQLException, IOException {
+boolean hasIndexId = table.getViewIndexId() != null;
+if (table.getType() != PTableType.VIEW && !hasIndexId) {
--- End diff --
Just curious - why does the viewIndexId determine whether or not the table
already exists?
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191936918
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/coprocessor/MetaDataEndpointImplTest.java
---
@@ -0,0 +1,299 @@
+package org.apache.phoenix.coprocessor;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.fail;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.SQLException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.HTable;
+import org.apache.phoenix.end2end.ParallelStatsDisabledIT;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData;
+import org.apache.phoenix.schema.PColumn;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.TableNotFoundException;
+import org.apache.phoenix.util.PhoenixRuntime;
+import org.junit.Test;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+
+/**
+ * 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.
+ */
+public class MetaDataEndpointImplTest extends ParallelStatsDisabledIT {
--- End diff --
One test that would be useful (and maybe you already have it?) is to create
a table, create a view (maybe on a different RS). Then drop the table and
recreate it and the view with the same name but different columns. Make sure
that the lazy cleanup code cleaned up the left over state correctly.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191936296
--- Diff:
phoenix-core/src/test/java/org/apache/phoenix/compile/QueryCompilerTest.java ---
@@ -1253,7 +1253,9 @@ public void testUnknownColumnInPKConstraint() throws
Exception {
}
}
-
+
+// see PHOENIX-3534, now tables can have duplicate columns and they
are removed implicitly
--- End diff --
This should still be detected with an exception thrown.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191935338
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/schema/PColumnImpl.java ---
@@ -156,14 +209,14 @@ public String toString() {
return (familyName == null ? "" : familyName.toString() +
QueryConstants.NAME_SEPARATOR) + name.toString();
}
-@Override
-public int hashCode() {
-final int prime = 31;
-int result = 1;
-result = prime * result + ((familyName == null) ? 0 :
familyName.hashCode());
-result = prime * result + ((name == null) ? 0 : name.hashCode());
-return result;
-}
+ @Override
+ public int hashCode() {
+ final int prime = 31;
+ int result = 1;
+ result = prime * result + ((familyName == null) ? 0 :
familyName.hashCode());
+ result = prime * result + ((name == null) ? 0 : name.hashCode());
+ return result;
+ }
--- End diff --
Minor nit - various formatting issues. Not sure if there are tabs now or if
the indenting was wrong before.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191933107
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2967,6 +2982,11 @@ protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection met
HTableDescriptor.SPLIT_POLICY + "='" +
SystemStatsSplitPolicy.class.getName() +"'"
);
}
+// TODO set the version for which the following upgrade code runs
correct
+if (currentServerSideTableTimeStamp <
MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_14_0) {
--- End diff --
Yes - just add a MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP_4_15_0
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191932416
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDatabaseMetaData.java
---
@@ -479,179 +498,355 @@ private void addTenantIdFilter(StringBuilder buf,
String tenantIdPattern) {
private static void appendConjunction(StringBuilder buf) {
buf.append(buf.length() == 0 ? "" : " and ");
}
-
-@Override
+
+private static final PColumnImpl TENANT_ID_COLUMN = new
PColumnImpl(PNameFactory.newName(TENANT_ID),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl TABLE_SCHEM_COLUMN = new
PColumnImpl(PNameFactory.newName(TABLE_SCHEM),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl TABLE_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(TABLE_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl COLUMN_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl DATA_TYPE_COLUMN = new
PColumnImpl(PNameFactory.newName(DATA_TYPE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl TYPE_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(TYPE_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(TYPE_NAME), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl COLUMN_SIZE_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_SIZE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, COLUMN_SIZE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl BUFFER_LENGTH_COLUMN = new
PColumnImpl(PNameFactory.newName(BUFFER_LENGTH),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(BUFFER_LENGTH), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl DECIMAL_DIGITS_COLUMN = new
PColumnImpl(PNameFactory.newName(DECIMAL_DIGITS),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
DECIMAL_DIGITS_BYTES, HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl NUM_PREC_RADIX_COLUMN = new
PColumnImpl(PNameFactory.newName(NUM_PREC_RADIX),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(NUM_PREC_RADIX), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl NULLABLE_COLUMN = new
PColumnImpl(PNameFactory.newName(NULLABLE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, NULLABLE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl REMARKS_COLUMN = new
PColumnImpl(PNameFactory.newName(REMARKS),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(REMARKS), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl COLUMN_DEF_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_DEF),
+ PNameFactory.newNam
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191931341
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixDatabaseMetaData.java
---
@@ -479,179 +498,355 @@ private void addTenantIdFilter(StringBuilder buf,
String tenantIdPattern) {
private static void appendConjunction(StringBuilder buf) {
buf.append(buf.length() == 0 ? "" : " and ");
}
-
-@Override
+
+private static final PColumnImpl TENANT_ID_COLUMN = new
PColumnImpl(PNameFactory.newName(TENANT_ID),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl TABLE_SCHEM_COLUMN = new
PColumnImpl(PNameFactory.newName(TABLE_SCHEM),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl TABLE_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(TABLE_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+private static final PColumnImpl COLUMN_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl DATA_TYPE_COLUMN = new
PColumnImpl(PNameFactory.newName(DATA_TYPE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, DATA_TYPE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl TYPE_NAME_COLUMN = new
PColumnImpl(PNameFactory.newName(TYPE_NAME),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(TYPE_NAME), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl COLUMN_SIZE_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_SIZE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, COLUMN_SIZE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl BUFFER_LENGTH_COLUMN = new
PColumnImpl(PNameFactory.newName(BUFFER_LENGTH),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(BUFFER_LENGTH), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl DECIMAL_DIGITS_COLUMN = new
PColumnImpl(PNameFactory.newName(DECIMAL_DIGITS),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
DECIMAL_DIGITS_BYTES, HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl NUM_PREC_RADIX_COLUMN = new
PColumnImpl(PNameFactory.newName(NUM_PREC_RADIX),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(NUM_PREC_RADIX), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl NULLABLE_COLUMN = new
PColumnImpl(PNameFactory.newName(NULLABLE),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PInteger.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false, NULLABLE_BYTES,
HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl REMARKS_COLUMN = new
PColumnImpl(PNameFactory.newName(REMARKS),
+ PNameFactory.newName(TABLE_FAMILY_BYTES),
PVarchar.INSTANCE, null, null, false, 1, SortOrder.getDefault(),
+ 0, null, false, null, false, false,
Bytes.toBytes(REMARKS), HConstants.LATEST_TIMESTAMP);
+ private static final PColumnImpl COLUMN_DEF_COLUMN = new
PColumnImpl(PNameFactory.newName(COLUMN_DEF),
+ PNameFactory.newNam
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191927956
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/exception/SQLExceptionCode.java
---
@@ -293,6 +293,7 @@ public SQLException newException(SQLExceptionInfo info)
{
SEQUENCE_NOT_CASTABLE_TO_AUTO_PARTITION_ID_COLUMN(1086, "44A17",
"Sequence Value not castable to auto-partition id column"),
CANNOT_COERCE_AUTO_PARTITION_ID(1087, "44A18", "Auto-partition id
cannot be coerced"),
+
--- End diff --
Revert please.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191926624
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/ViewIT.java
---
@@ -388,51 +435,65 @@ public void
testViewAndTableInDifferentSchemas(boolean isNamespaceMapped) throws
} catch (TableNotFoundException ignore) {
}
ddl = "DROP TABLE " + fullTableName1;
-validateCannotDropTableWithChildViewsWithoutCascade(conn,
fullTableName1);
ddl = "DROP VIEW " + fullViewName2;
conn.createStatement().execute(ddl);
ddl = "DROP TABLE " + fullTableName1;
conn.createStatement().execute(ddl);
}
-
+
@Test
-public void testDisallowDropOfColumnOnParentTable() throws Exception {
+public void testDropOfColumnOnParentTableInvalidatesView() throws
Exception {
Connection conn = DriverManager.getConnection(getUrl());
+String fullTableName = generateUniqueTableName();
+String viewName = generateUniqueViewName();
+splitSystemCatalog(Lists.newArrayList(fullTableName, viewName));
+
String ddl = "CREATE TABLE " + fullTableName + " (k1 INTEGER NOT
NULL, k2 INTEGER NOT NULL, v1 DECIMAL, CONSTRAINT pk PRIMARY KEY (k1, k2))" +
tableDDLOptions;
conn.createStatement().execute(ddl);
-String viewName = "V_" + generateUniqueName();
ddl = "CREATE VIEW " + viewName + "(v2 VARCHAR, v3 VARCHAR) AS
SELECT * FROM " + fullTableName + " WHERE v1 = 1.0";
conn.createStatement().execute(ddl);
-try {
-conn.createStatement().execute("ALTER TABLE " + fullTableName
+ " DROP COLUMN v1");
-fail();
-} catch (SQLException e) {
-
assertEquals(SQLExceptionCode.CANNOT_MUTATE_TABLE.getErrorCode(),
e.getErrorCode());
+conn.createStatement().execute("ALTER TABLE " + fullTableName + "
DROP COLUMN v1");
+// TODO see if its possibel to prevent the dropping of a column
thats required by a child view (for its view where clause)
+// the view should be invalid
--- End diff --
I think it's fine to consider the view invalid (i.e. fail any query that
attempts to use it) if all it's columns can no longer be found. This is pretty
typical in RDBMS.
---
[GitHub] phoenix pull request #303: PHOENIX-3534 Support multi region SYSTEM.CATALOG ...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/303#discussion_r191925360
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ExplainPlanWithStatsEnabledIT.java
---
@@ -1202,33 +1202,44 @@ private void
testUseStatsForParallelizationOnSaltedTable(boolean useStatsFlag, b
assertEquals("B", rs.getString(1));
}
- @Test
- public void testUseStatsForParallelizationProperyOnViewIndex() throws
SQLException {
- String tableName = generateUniqueName();
- String viewName = generateUniqueName();
- String tenantViewName = generateUniqueName();
- String viewIndexName = generateUniqueName();
- boolean useStats = !DEFAULT_USE_STATS_FOR_PARALLELIZATION;
- try (Connection conn = DriverManager.getConnection(getUrl())) {
- conn.createStatement()
- .execute("create table " + tableName
- + "(tenantId CHAR(15)
NOT NULL, pk1 integer NOT NULL, v varchar CONSTRAINT PK PRIMARY KEY "
- + "(tenantId, pk1))
MULTI_TENANT=true");
- try (Connection tenantConn =
getTenantConnection("tenant1")) {
- conn.createStatement().execute("CREATE VIEW " +
viewName + " AS SELECT * FROM " + tableName);
- conn.createStatement().execute("CREATE INDEX "
+ viewIndexName + " on " + viewName + " (v) ");
- tenantConn.createStatement().execute("CREATE
VIEW " + tenantViewName + " AS SELECT * FROM " + viewName);
- conn.createStatement()
- .execute("ALTER TABLE " +
tableName + " set USE_STATS_FOR_PARALLELIZATION=" + useStats);
- // fetch the latest view ptable
- PhoenixRuntime.getTableNoCache(tenantConn,
viewName);
- PhoenixConnection phxConn =
conn.unwrap(PhoenixConnection.class);
- PTable viewIndex = phxConn.getTable(new
PTableKey(phxConn.getTenantId(), viewIndexName));
- assertEquals("USE_STATS_FOR_PARALLELIZATION
property set incorrectly", useStats,
- PhoenixConfigurationUtil
-
.getStatsForParallelizationProp(tenantConn.unwrap(PhoenixConnection.class),
viewIndex));
- }
- }
- }
+@Test
+public void testUseStatsForParallelizationProperyOnViewIndex() throws
SQLException {
+String tableName = generateUniqueName();
+String viewName = generateUniqueName();
+String tenantViewName = generateUniqueName();
+String viewIndexName = generateUniqueName();
+boolean useStats = !DEFAULT_USE_STATS_FOR_PARALLELIZATION;
+try (Connection conn = DriverManager.getConnection(getUrl())) {
+conn.createStatement()
+.execute("create table " + tableName
++ "(tenantId CHAR(15) NOT NULL, pk1 integer
NOT NULL, v varchar CONSTRAINT PK PRIMARY KEY "
++ "(tenantId, pk1)) MULTI_TENANT=true");
+try (Connection tenantConn = getTenantConnection("tenant1")) {
+conn.createStatement().execute("CREATE VIEW " + viewName +
" AS SELECT * FROM " + tableName);
+conn.createStatement().execute("CREATE INDEX " +
viewIndexName + " on " + viewName + " (v) ");
+tenantConn.createStatement().execute("CREATE VIEW " +
tenantViewName + " AS SELECT * FROM " + viewName);
+conn.createStatement()
+.execute("ALTER TABLE " + tableName + " set
USE_STATS_FOR_PARALLELIZATION=" + useStats);
+// changing a property on a base table does not change the
property on a view
--- End diff --
Shouldn't setting a property on the base table impact the view as well? In
this case, USE_STATS_FOR_PARALLELIZATION only makes sense to set on a physical
table. I think we only look it up from a physical table as well, so this is
somewhat moot, but in general, I'd think that setting a property on a base
table should be seen by it's views if the property has not been set there.
---
[GitHub] phoenix pull request #301: PHOENIX-4728 The upsert select must project tuple...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/301#discussion_r190381688
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/compile/UpsertCompiler.java ---
@@ -549,7 +549,7 @@ public MutationPlan compile(UpsertStatement upsert)
throws SQLException {
select = SelectStatement.create(select, hint);
// Pass scan through if same table in upsert and select so
that projection is computed correctly
// Use optimizer to choose the best plan
-QueryCompiler compiler = new QueryCompiler(statement, select,
selectResolver, targetColumns, parallelIteratorFactoryToBe, new
SequenceManager(statement), false, false, null);
+QueryCompiler compiler = new QueryCompiler(statement, select,
selectResolver, targetColumns, parallelIteratorFactoryToBe, new
SequenceManager(statement), true, false, null);
--- End diff --
This seems like too general of a change for the specific issue you're
trying to fix for ARRAY_APPEND. I'm also not sure *why* it would impact it.
Can't you make changes to ArrayAppendFunction or it's base class to get the
desired affect?
Any opinions, @maryannxue. Do you remember when/why we need this
projectTuples boolean for QueryCompiler?
---
[GitHub] phoenix pull request #300: Omid transaction support in Phoenix
GitHub user JamesRTaylor opened a pull request: https://github.com/apache/phoenix/pull/300 Omid transaction support in Phoenix You can merge this pull request into a Git repository by running: $ git pull https://github.com/ohadshacham/phoenix 4.x-HBase-1.3-Omid-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/300.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 #300 commit 1039f9d4ca0da5185d8babaa6681459e07da61ad Author: Ohad Shacham Date: 2018-04-03T14:03:55Z Add Omid support for Phoenix commit a11bddcacb90d553f6c0fdab37e2c335f7f23ac4 Author: Ohad Shacham Date: 2018-05-02T11:55:47Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 45e6b0e309d037c03221de249501fa7c4bc651db Author: Ohad Shacham Date: 2018-05-08T07:16:19Z Remove hard coded Omid commit 18bea3907ef73c6f842ac5410ea4623be5a36b18 Author: Ohad Shacham Date: 2018-05-08T09:28:05Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 38ab6f459e6174e150d88a146b4a35d9c1857fb6 Author: Ohad Shacham Date: 2018-05-15T08:19:05Z some merge fixes commit cbab9b72ee5c62d9512b9472b010f581e3866e21 Author: Ohad Shacham Date: 2018-05-22T13:04:56Z Fix hbase config commit 7c834994a862f5e2a0edfa560c0a1c4f047383ca Author: Ohad Shacham Date: 2018-05-22T18:30:01Z Partially revert the following commits. https://github.com/ohadshacham/phoenix/commit/45e6b0e309d037c03221de249501fa7c4bc651db https://github.com/ohadshacham/phoenix/commit/38ab6f459e6174e150d88a146b4a35d9c1857fb6 commit dc6de3ab12ad18e9dc5622521bfaf5a662e4d409 Author: Ohad Shacham Date: 2018-05-22T18:32:35Z Merge commit '2015345a023f0adb59174443ec1328bb1399f11b' into 4.x-HBase-1.3-Omid-2 commit 9eae4abdbc770381620e23f4793acb279cec2544 Author: Ohad Shacham Date: 2018-05-22T18:42:26Z Change back to TEPHRA what needed for testing TEPHRA commit 0c5dfd642b3738d2d09aa19f2e6d4cbb97852c20 Author: Ohad Shacham Date: 2018-05-23T12:14:37Z remove unnecessary changes. ---
[GitHub] phoenix pull request #299: 4.x h base 1.3 omid 2
Github user JamesRTaylor closed the pull request at: https://github.com/apache/phoenix/pull/299 ---
[GitHub] phoenix pull request #299: 4.x h base 1.3 omid 2
GitHub user JamesRTaylor opened a pull request: https://github.com/apache/phoenix/pull/299 4.x h base 1.3 omid 2 You can merge this pull request into a Git repository by running: $ git pull https://github.com/ohadshacham/phoenix 4.x-HBase-1.3-Omid-2 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/phoenix/pull/299.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 #299 commit 1039f9d4ca0da5185d8babaa6681459e07da61ad Author: Ohad Shacham Date: 2018-04-03T14:03:55Z Add Omid support for Phoenix commit a11bddcacb90d553f6c0fdab37e2c335f7f23ac4 Author: Ohad Shacham Date: 2018-05-02T11:55:47Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 45e6b0e309d037c03221de249501fa7c4bc651db Author: Ohad Shacham Date: 2018-05-08T07:16:19Z Remove hard coded Omid commit 18bea3907ef73c6f842ac5410ea4623be5a36b18 Author: Ohad Shacham Date: 2018-05-08T09:28:05Z Merge remote-tracking branch 'upstream/4.x-HBase-1.3' into 4.x-HBase-1.3-Omid-2 commit 38ab6f459e6174e150d88a146b4a35d9c1857fb6 Author: Ohad Shacham Date: 2018-05-15T08:19:05Z some merge fixes commit cbab9b72ee5c62d9512b9472b010f581e3866e21 Author: Ohad Shacham Date: 2018-05-22T13:04:56Z Fix hbase config commit 7c834994a862f5e2a0edfa560c0a1c4f047383ca Author: Ohad Shacham Date: 2018-05-22T18:30:01Z Partially revert the following commits. https://github.com/ohadshacham/phoenix/commit/45e6b0e309d037c03221de249501fa7c4bc651db https://github.com/ohadshacham/phoenix/commit/38ab6f459e6174e150d88a146b4a35d9c1857fb6 commit dc6de3ab12ad18e9dc5622521bfaf5a662e4d409 Author: Ohad Shacham Date: 2018-05-22T18:32:35Z Merge commit '2015345a023f0adb59174443ec1328bb1399f11b' into 4.x-HBase-1.3-Omid-2 commit 9eae4abdbc770381620e23f4793acb279cec2544 Author: Ohad Shacham Date: 2018-05-22T18:42:26Z Change back to TEPHRA what needed for testing TEPHRA ---
[GitHub] phoenix pull request #298: PHOENIX-4666 Persistent subquery cache for hash j...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/298#discussion_r187393698
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/cache/ServerCacheClient.java ---
@@ -216,22 +234,146 @@ public void close() throws SQLException {
}
}
}
-
+}
+
+public ServerCache checkServerCache(final byte[] cacheId, ScanRanges
keyRanges, final TableRef cacheUsingTableRef,
--- End diff --
FYI, we already have a mechanism for this. If the server throws a
HashJoinCacheNotFoundException, then the client will react by sending the hash
cache to the region servers that don't have it.
---
[GitHub] phoenix issue #297: PHOENIX-4575: Phoenix metadata KEEP_DELETED_CELLS and VE...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/297 +1. Thanks for taking this to the finish line, @ChinmaySKulkarni. Is this on top of your patch for PHOENIX-4579? What order should I commit them? ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 +1. Nice work, @ChinmaySKulkarni! I'll get this committed. Might need some help with a patch that applied to 5.x-HBase-2.0 branch. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 If possible, we shouldn't modify SYSTEM table metadata after the HBase table has been created even if auto-upgrade is enabled. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Thanks for the updates, @ChinmaySKulkarni. Looks like you still need to rebase it. Also, one test that would be really valuable would be to: * disable auto upgrade * run execute upgrade * make some change to the HBase metadata of SYSTEM.CATALOG through ALTER TABLE or direct HBase API call (for example, set VERSIONS to some value). * connect a client that create a new ConnectionQueryServices instance to be created(see UpdateCacheAcrossDifferentClientsIT for how to do that) * ensure that the metadata change to SYSTEM.CATALOG is not undone. * do the same with auto upgrade enabled (which might cause the change to be reverted which is ok - this is more for making sure we understand this flow in both cases) This test will ensure that when this hits production, if we have auto upgrade disabled, we can make manual changes to the SYSTEM.CATALOG. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 This looks good. Couple of minor things for the tests, and then rebase it and I think we'll be good to go. Nice work, @ChinmaySKulkarni. ---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r180946851
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
---
@@ -0,0 +1,577 @@
+/*
+ * 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.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.exception.UpgradeRequiredException;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.*;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.UpgradeUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.*;
+import java.util.concurrent.TimeoutException;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SystemCatalogCreationOnConnectionIT {
+private HBaseTestingUtility testUtil = null;
+private Set hbaseTables;
+private static boolean setOldTimestampToInduceUpgrade = false;
+private static int countUpgradeAttempts;
+// This flag is used to figure out if the SYSCAT schema was actually
upgraded or not, based on the timestamp of SYSCAT
+// (different from an upgrade attempt)
+private static int actualSysCatUpgrades;
+private static final String PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG =
"SYSTEM:CATALOG";
+private static final String PHOENIX_SYSTEM_CATALOG = "SYSTEM.CATALOG";
+private static final String EXECUTE_UPGRADE_COMMAND = "EXECUTE
UPGRADE";
+
+private static final Set PHOENIX_SYSTEM_TABLES = new
HashSet<>(Arrays.asList(
+ "SYSTEM.CATALOG", "SYSTEM.SEQUENCE", "SYSTEM.STATS",
"SYSTEM.FUNCTION",
+ "SYSTEM.MUTEX"));
+
+private static final Set
PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(
+ Arrays.asList("SYSTEM:CATALOG", "SYSTEM:SEQUENCE", "SYSTEM:STATS",
"SYSTEM:FUNCTION",
+"SYSTEM:MUTEX"));
+
+private static class PhoenixSysCatCreationServices extends
ConnectionQueryServicesImpl {
+
+public PhoenixSysCatCreationServices(QueryServices services,
PhoenixEmbeddedDriver.ConnectionInfo connectionInfo, Properties info) {
+super(services, connectionInfo, info);
+}
+
+@Override
+protected void setUpgradeRequired() {
+super.setUpgradeRequired();
+countUpgradeAttempts++;
+}
+
+@Override
+protected long getSystemTableVersion() {
+if (setOldTimestampToInduceUpgrade) {
+// Return the next lower version where an upgrade was
performed to induce setting the upgradeRequired flag
+return MetaDataProtocol.getPriorUpgradeVersion();
+}
+return MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+}
+
+@Override
+protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection metaConnection,
+ long currentServerSideTableTimeStamp) throws
InterruptedException, SQLException, TimeoutException, IOException {
+PhoenixConnection newMetaConnection =
super.upgradeSystemCatalo
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r180945785
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
---
@@ -0,0 +1,577 @@
+/*
+ * 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.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.exception.UpgradeRequiredException;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.*;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.UpgradeUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.*;
+import java.util.concurrent.TimeoutException;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SystemCatalogCreationOnConnectionIT {
+private HBaseTestingUtility testUtil = null;
+private Set hbaseTables;
+private static boolean setOldTimestampToInduceUpgrade = false;
+private static int countUpgradeAttempts;
+// This flag is used to figure out if the SYSCAT schema was actually
upgraded or not, based on the timestamp of SYSCAT
+// (different from an upgrade attempt)
+private static int actualSysCatUpgrades;
+private static final String PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG =
"SYSTEM:CATALOG";
+private static final String PHOENIX_SYSTEM_CATALOG = "SYSTEM.CATALOG";
+private static final String EXECUTE_UPGRADE_COMMAND = "EXECUTE
UPGRADE";
+
+private static final Set PHOENIX_SYSTEM_TABLES = new
HashSet<>(Arrays.asList(
+ "SYSTEM.CATALOG", "SYSTEM.SEQUENCE", "SYSTEM.STATS",
"SYSTEM.FUNCTION",
+ "SYSTEM.MUTEX"));
+
+private static final Set
PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(
+ Arrays.asList("SYSTEM:CATALOG", "SYSTEM:SEQUENCE", "SYSTEM:STATS",
"SYSTEM:FUNCTION",
+"SYSTEM:MUTEX"));
+
+private static class PhoenixSysCatCreationServices extends
ConnectionQueryServicesImpl {
+
+public PhoenixSysCatCreationServices(QueryServices services,
PhoenixEmbeddedDriver.ConnectionInfo connectionInfo, Properties info) {
+super(services, connectionInfo, info);
+}
+
+@Override
+protected void setUpgradeRequired() {
+super.setUpgradeRequired();
+countUpgradeAttempts++;
+}
+
+@Override
+protected long getSystemTableVersion() {
+if (setOldTimestampToInduceUpgrade) {
+// Return the next lower version where an upgrade was
performed to induce setting the upgradeRequired flag
+return MetaDataProtocol.getPriorUpgradeVersion();
+}
+return MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+}
+
+@Override
+protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection metaConnection,
+ long currentServerSideTableTimeStamp) throws
InterruptedException, SQLException, TimeoutException, IOException {
+PhoenixConnection newMetaConnection =
super.upgradeSystemCatalo
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r180945611
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
---
@@ -0,0 +1,577 @@
+/*
+ * 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.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.exception.UpgradeRequiredException;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.*;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.UpgradeUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.*;
+import java.util.concurrent.TimeoutException;
+
+import static org.junit.Assert.*;
+
+@Category(NeedsOwnMiniClusterTest.class)
+public class SystemCatalogCreationOnConnectionIT {
+private HBaseTestingUtility testUtil = null;
+private Set hbaseTables;
+private static boolean setOldTimestampToInduceUpgrade = false;
+private static int countUpgradeAttempts;
+// This flag is used to figure out if the SYSCAT schema was actually
upgraded or not, based on the timestamp of SYSCAT
+// (different from an upgrade attempt)
+private static int actualSysCatUpgrades;
+private static final String PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG =
"SYSTEM:CATALOG";
+private static final String PHOENIX_SYSTEM_CATALOG = "SYSTEM.CATALOG";
+private static final String EXECUTE_UPGRADE_COMMAND = "EXECUTE
UPGRADE";
+
+private static final Set PHOENIX_SYSTEM_TABLES = new
HashSet<>(Arrays.asList(
+ "SYSTEM.CATALOG", "SYSTEM.SEQUENCE", "SYSTEM.STATS",
"SYSTEM.FUNCTION",
+ "SYSTEM.MUTEX"));
+
+private static final Set
PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(
+ Arrays.asList("SYSTEM:CATALOG", "SYSTEM:SEQUENCE", "SYSTEM:STATS",
"SYSTEM:FUNCTION",
+"SYSTEM:MUTEX"));
+
+private static class PhoenixSysCatCreationServices extends
ConnectionQueryServicesImpl {
+
+public PhoenixSysCatCreationServices(QueryServices services,
PhoenixEmbeddedDriver.ConnectionInfo connectionInfo, Properties info) {
+super(services, connectionInfo, info);
+}
+
+@Override
+protected void setUpgradeRequired() {
+super.setUpgradeRequired();
+countUpgradeAttempts++;
+}
+
+@Override
+protected long getSystemTableVersion() {
+if (setOldTimestampToInduceUpgrade) {
+// Return the next lower version where an upgrade was
performed to induce setting the upgradeRequired flag
+return MetaDataProtocol.getPriorUpgradeVersion();
+}
+return MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+}
+
+@Override
+protected PhoenixConnection
upgradeSystemCatalogIfRequired(PhoenixConnection metaConnection,
+ long currentServerSideTableTimeStamp) throws
InterruptedException, SQLException, TimeoutException, IOException {
+PhoenixConnection newMetaConnection =
super.upgradeSystemCatalo
[GitHub] phoenix issue #296: PHOENIX-4668: Remove unnecessary table descriptor modifi...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/296 +1. Will commit soon. ---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r179000414
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2643,6 +2661,26 @@ public void upgradeSystemTables(final String url,
final Properties props) throws
metaConnection.setRunningUpgrade(true);
try {
metaConnection.createStatement().executeUpdate(QueryConstants.CREATE_TABLE_METADATA);
+
+// HBase Namespace SYSTEM is created by {@link
ensureSystemTablesMigratedToSystemNamespace(ReadOnlyProps)} method
+// This statement will create its entry in SYSCAT table,
so that GRANT/REVOKE commands can work
+// with SYSTEM Namespace. (See PHOENIX-4227
https://issues.apache.org/jira/browse/PHOENIX-4227)
+if (SchemaUtil.isNamespaceMappingEnabled(PTableType.SYSTEM,
+ ConnectionQueryServicesImpl.this.getProps())) {
--- End diff --
If autoUpgrade is enabled, we don't want the user to have to run EXECUTE
UPGRADE. Move the creation of the namespace to ensureTableCreated if necessary
(but keep the ensureSystemTablesMigratedToSystemNamespace logic in
upgradeSystemTables). Let's have a test for this too.
I think it'd be better to have the acquireUpgradeMutex only in
upgradeSystemTables so we only do this once. We can acquire it before calling
ensureSystemTablesMigratedToSystemNamespace.
---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r178935257
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2643,6 +2661,26 @@ public void upgradeSystemTables(final String url,
final Properties props) throws
metaConnection.setRunningUpgrade(true);
try {
metaConnection.createStatement().executeUpdate(QueryConstants.CREATE_TABLE_METADATA);
+
+// HBase Namespace SYSTEM is created by {@link
ensureSystemTablesMigratedToSystemNamespace(ReadOnlyProps)} method
+// This statement will create its entry in SYSCAT table,
so that GRANT/REVOKE commands can work
+// with SYSTEM Namespace. (See PHOENIX-4227
https://issues.apache.org/jira/browse/PHOENIX-4227)
+if (SchemaUtil.isNamespaceMappingEnabled(PTableType.SYSTEM,
+ ConnectionQueryServicesImpl.this.getProps())) {
--- End diff --
Inside of this block might be a good place to call
ensureSystemTablesMigratedToSystemNamespace. We can treat this as an upgrade
step in the 4.14 upgrade path.
---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r178936752
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java
---
@@ -0,0 +1,533 @@
+/*
+ * 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.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.phoenix.coprocessor.MetaDataProtocol;
+import org.apache.phoenix.exception.SQLExceptionCode;
+import org.apache.phoenix.exception.UpgradeRequiredException;
+import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver;
+import org.apache.phoenix.jdbc.PhoenixTestDriver;
+import org.apache.phoenix.query.*;
+import org.apache.phoenix.util.ReadOnlyProps;
+import org.apache.phoenix.util.UpgradeUtil;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.*;
+
+import static org.junit.Assert.*;
+
+public class SystemCatalogCreationOnConnectionIT {
+private HBaseTestingUtility testUtil = null;
+private Set hbaseTables;
+private static boolean setOldTimestampToInduceUpgrade = false;
+private static int countUpgradeAttempts;
+private static final String PHOENIX_NAMESPACE_MAPPED_SYSTEM_CATALOG =
"SYSTEM:CATALOG";
+private static final String PHOENIX_SYSTEM_CATALOG = "SYSTEM.CATALOG";
+private static final String EXECUTE_UPGRADE_COMMAND = "EXECUTE
UPGRADE";
+
+private static final Set PHOENIX_SYSTEM_TABLES = new
HashSet<>(Arrays.asList(
+ "SYSTEM.CATALOG", "SYSTEM.SEQUENCE", "SYSTEM.STATS",
"SYSTEM.FUNCTION",
+ "SYSTEM.MUTEX"));
+
+private static final Set
PHOENIX_NAMESPACE_MAPPED_SYSTEM_TABLES = new HashSet<>(
+ Arrays.asList("SYSTEM:CATALOG", "SYSTEM:SEQUENCE", "SYSTEM:STATS",
"SYSTEM:FUNCTION",
+"SYSTEM:MUTEX"));
+
+private static class PhoenixSysCatCreationServices extends
ConnectionQueryServicesImpl {
+
+public PhoenixSysCatCreationServices(QueryServices services,
PhoenixEmbeddedDriver.ConnectionInfo connectionInfo, Properties info) {
+super(services, connectionInfo, info);
+}
+
+@Override
+protected void setUpgradeRequired() {
+super.setUpgradeRequired();
+countUpgradeAttempts++;
+}
+
+@Override
+protected long getSystemTableVersion() {
+if (setOldTimestampToInduceUpgrade) {
+// Return the next lower version where an upgrade was
performed to induce setting the upgradeRequired flag
+return MetaDataProtocol.getPriorUpgradeVersion();
+}
+return MetaDataProtocol.MIN_SYSTEM_TABLE_TIMESTAMP;
+}
+}
+
+public static class PhoenixSysCatCreationTestingDriver extends
PhoenixTestDriver {
+private ConnectionQueryServices cqs;
+private final ReadOnlyProps overrideProps;
+
+public PhoenixSysCatCreationTestingDriver(ReadOnlyProps props) {
+overrideProps = props;
+}
+
+@Override // public for testing
+public synchronized ConnectionQueryServices
getConnectionQueryServices(String url, Properties info) throws SQLException {
+if (cqs == null) {
+cqs = new PhoenixSysCatCreationServices(new
QueryServicesTestImpl(getDefaultProps(), overrideProps),
ConnectionInfo.create(url), info);
+cqs.init(url, info);
+
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/295#discussion_r178933517 --- Diff: phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java --- @@ -1039,10 +1065,20 @@ private HTableDescriptor ensureTableCreated(byte[] physicalTableName, PTableType } } +if (isMetaTable && SchemaUtil.isNamespaceMappingEnabled(PTableType.SYSTEM, --- End diff -- The ensureSystemTablesMigratedToSystemNamespace has race conditions in the case of multiple clients executing it at the same time. We should treat this as an upgrade step and do it while we have the SYSTEM.MUTEX lock (which ensures only a single client will be executing upgrade code). ---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request: https://github.com/apache/phoenix/pull/295#discussion_r178936507 --- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/SystemCatalogCreationOnConnectionIT.java --- @@ -0,0 +1,533 @@ +/* + * 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.hadoop.conf.Configuration; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.HConstants; +import org.apache.hadoop.hbase.TableName; +import org.apache.phoenix.coprocessor.MetaDataProtocol; +import org.apache.phoenix.exception.SQLExceptionCode; +import org.apache.phoenix.exception.UpgradeRequiredException; +import org.apache.phoenix.jdbc.PhoenixEmbeddedDriver; +import org.apache.phoenix.jdbc.PhoenixTestDriver; +import org.apache.phoenix.query.*; +import org.apache.phoenix.util.ReadOnlyProps; +import org.apache.phoenix.util.UpgradeUtil; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +import java.io.IOException; +import java.sql.Connection; +import java.sql.SQLException; +import java.util.*; + +import static org.junit.Assert.*; + --- End diff -- You'll need to add a class-level annotation here so that this test runs when mvn verify is run: @Category(NeedsOwnMiniClusterTest.class) ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 I think it's ok to make these changes. Not a problem to make DO_NOT_UPGRADE public if need be. Feel free to add new arguments if necessary to ensureTableCreated. I think any special logic around SYSTEM.CATALOG or SYSTEM:CATALOG is ok as long as it's isolated. ---
[GitHub] phoenix issue #294: PHOENIX-4643 Implement ARRAY_REMOVE built in function
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/294 +1. Thanks for the contribution, @xjodoin. I'll commit this to the various branches. ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Thinking more, I think itâs fine to go through the CREATE TABLE code path and move the logic in the try block of init down into ensureTableCreated. Itâs only called once on the first connection made to a cluster from a client. Keeping the code simple and doing a single RPC to get the metadata and another to get the version will outweigh the minor overhead of compiling CREATE TABLE (plus like you mentioned before, we need to do that to build up the args for ensureTableCreated anyway). ---
[GitHub] phoenix issue #295: PHOENIX-4579: Add a config to conditionally create Phoen...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Ok, sounds like a good plan. ---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r176783002
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2405,16 +2413,26 @@ public Void call() throws Exception {
openConnection();
hConnectionEstablished = true;
boolean isDoNotUpgradePropSet =
UpgradeUtil.isNoUpgradeSet(props);
+boolean doesSystemCatalogAlreadyExist = false;
--- End diff --
Good points. How about if we leave the ensureTableCreated call where it is,
but move the logic in that try block of the init method into the
ensureTableCreated method so itâs consolidated?
---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r176520222
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2405,16 +2413,26 @@ public Void call() throws Exception {
openConnection();
hConnectionEstablished = true;
boolean isDoNotUpgradePropSet =
UpgradeUtil.isNoUpgradeSet(props);
+boolean doesSystemCatalogAlreadyExist = false;
--- End diff --
I'd also recommend committing PHOENIX-4668 and PHOENIX-4575 first as
that'll simplify what you need to do here a bit.
---
[GitHub] phoenix pull request #295: PHOENIX-4579: Add a config to conditionally creat...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/295#discussion_r176506809
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java
---
@@ -2405,16 +2413,26 @@ public Void call() throws Exception {
openConnection();
hConnectionEstablished = true;
boolean isDoNotUpgradePropSet =
UpgradeUtil.isNoUpgradeSet(props);
+boolean doesSystemCatalogAlreadyExist = false;
--- End diff --
The flow of this code is very confusing. Let's do the compatibility check
only once in ensureTableCreated by making the following changes:
- do not call ensureTableCreated from createTable if the table is a system
table
- call ensureTableCreated instead here and remove this entire try block
- do all checks required in ensureTableCreated where we already determine
if the table exists or not
- in ensureTableCreated, if SYSTEM.CATALOG doesn't exist and
!isAutoUpgradeEnabled or isDoNotUpgradePropSet, then throw our standard
UpgradeRequiredException immediately without creating system catalog metadata.
- the only call to checkClientServerCompatibility should be in
ensureTableCreated (when isMetaTable is true)
- make sure to be defensive in the creation of the SYSTEM namespace and
moving of SYSTEM tables as it's possible that multiple clients may be
attempting to do that.
I think this improve the maintainability of this code (and fix this issue
too).
---
[GitHub] phoenix issue #295: WIP: Added check for system catalog timestamp while doin...
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/295 Please amend your commit message to be in the form: PHOENIX-4579 That way, PR comments will automatically appear as JIRA comments. ---
[GitHub] phoenix issue #294: PHOENIX-4643 Implement ARRAY_REMOVE built in function
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/294 Looks good - one minor nit and then I think we'll be good to go. ---
[GitHub] phoenix pull request #294: PHOENIX-4643 Implement ARRAY_REMOVE built in func...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/294#discussion_r175620626
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayRemoveFunctionIT.java
---
@@ -0,0 +1,383 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+
+import org.apache.phoenix.schema.TypeMismatchException;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ArrayRemoveFunctionIT extends ParallelStatsDisabledIT {
+
+ private Connection conn;
+ private String tableName;
+
+ @Before
+ public void setup() throws Exception {
+ conn = DriverManager.getConnection(getUrl());
+ tableName = initTables(conn);
+ }
+
+ private String initTables(Connection conn) throws Exception {
+ String tableName = generateUniqueName();
+ String ddl = "CREATE TABLE " + tableName
+ + " (region_name VARCHAR PRIMARY KEY,varchars
VARCHAR[],integers INTEGER[],doubles DOUBLE[],bigints BIGINT[],"
+ + "chars CHAR(15)[],double1 DOUBLE,char1
CHAR(17),nullcheck INTEGER,chars2 CHAR(15)[], nullVarchar VARCHAR[], nullBigInt
BIGINT[],double2 DOUBLE,integer1 INTEGER,oneItem VARCHAR[],char2
char(15),varchar1 VARCHAR)";
+ conn.createStatement().execute(ddl);
+ String dml = "UPSERT INTO " + tableName
+ +
"(region_name,varchars,integers,doubles,bigints,chars,double1,char1,nullcheck,chars2,double2,integer1,oneItem,char2,varchar1)
VALUES('SF Bay Area',"
+ + "ARRAY['2345','46345','23234']," +
"ARRAY[2345,46345,23234,456],"
+ + "ARRAY[10.0,23.45,46.345,23.234,45.6,5.78],"
+ "ARRAY[12,34,56,78,910],"
+ + "ARRAY['a','','c','ddd','e','c']," +
"23.45," + "'wert'," + "NULL,"
+ + "ARRAY['a','','c','ddd','e','foo']," +
"12," + "12," + "ARRAY['alone'],'2345','')";
+ PreparedStatement stmt = conn.prepareStatement(dml);
+ stmt.execute();
+ conn.commit();
+ return tableName;
+ }
+
+ @Test
+ public void testEmptyArrayModification() throws Exception {
+ ResultSet rs = conn.createStatement()
+ .executeQuery("SELECT
ARRAY_REMOVE(nullVarChar,'34567') FROM " + tableName + " LIMIT 1");
+ assertTrue(rs.next());
+
+ assertNull(rs.getArray(1));
+ assertFalse(rs.next());
+ }
+
+ @Test
+ public void testArrayRemoveFunctionVarchar() throws Exception {
+ ResultSet rs = conn.createStatement().executeQuery(
+ "SELECT ARRAY_REMOVE(varchars,'23234') FROM " +
tableName + " WHERE region_name = 'SF Bay Area'");
+ assertTrue(rs.next());
+
+ assertEquals(conn.createArrayOf("VARCHAR", new String[] {
"2345", "46345" }), rs.getArray(1));
+ assertFalse(rs.next());
+ }
+
+ @Test
+ public void testArrayRemoveFunctionInteger() throws Exception {
+
[GitHub] phoenix pull request #294: PHOENIX-4643 Implement ARRAY_REMOVE built in func...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/294#discussion_r175225740
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayRemoveFunctionIT.java
---
@@ -0,0 +1,425 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Array;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+
+import org.apache.phoenix.schema.TypeMismatchException;
+import org.junit.Test;
+
+public class ArrayRemoveFunctionIT extends ParallelStatsDisabledIT {
+private String initTables(Connection conn) throws Exception {
+String tableName = generateUniqueName();
+String ddl = "CREATE TABLE " + tableName
++ " (region_name VARCHAR PRIMARY KEY,varchars
VARCHAR[],integers INTEGER[],doubles DOUBLE[],bigints BIGINT[],"
++ "chars CHAR(15)[],double1 DOUBLE,char1
CHAR(17),nullcheck INTEGER,chars2 CHAR(15)[], nullVarchar VARCHAR[], nullBigInt
BIGINT[],double2 DOUBLE,integer1 INTEGER)";
+conn.createStatement().execute(ddl);
+String dml = "UPSERT INTO " + tableName +
"(region_name,varchars,integers,doubles,bigints,chars,double1,char1,nullcheck,chars2,double2,integer1)
VALUES('SF Bay Area'," +
+"ARRAY['2345','46345','23234']," +
+"ARRAY[2345,46345,23234,456]," +
+"ARRAY[23.45,46.345,23.234,45.6,5.78]," +
+"ARRAY[12,34,56,78,910]," +
+"ARRAY['a','','c','ddd','e']," +
+"23.45," +
+"'wert'," +
+"NULL," +
+"ARRAY['a','','c','ddd','e','foo']," +
+"12,"+
+"12"+
+")";
+PreparedStatement stmt = conn.prepareStatement(dml);
+stmt.execute();
+conn.commit();
+return tableName;
+}
+
+@Test
+public void testEmptyArrayModification() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String tableName = initTables(conn);
+
+ResultSet rs;
+rs = conn.createStatement().executeQuery("SELECT
ARRAY_REMOVE(nullVarChar,'34567') FROM " + tableName + " LIMIT 1");
+assertTrue(rs.next());
+
+assertNull(rs.getArray(1));
+assertFalse(rs.next());
+}
+
+@Test
+public void testArrayRemoveFunctionVarchar() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String tableName = initTables(conn);
+
+ResultSet rs;
+rs = conn.createStatement().executeQuery("SELECT
ARRAY_REMOVE(varchars,'23234') FROM " + tableName + " WHERE region_name = 'SF
Bay Area'");
+assertTrue(rs.next());
+
+String[] strings = new String[]{"2345", "46345"};
+
+Array array = conn.createArrayOf("VARCHAR", strings);
+
+assertEquals(array, rs.getArray(1));
+assertFalse(rs.next());
+}
+
+@Test
+public void testArrayRemoveFunctionInteger() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String t
[GitHub] phoenix pull request #294: PHOENIX-4643 Implement ARRAY_REMOVE built in func...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/294#discussion_r175225898
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayRemoveFunctionIT.java
---
@@ -0,0 +1,425 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Array;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+
+import org.apache.phoenix.schema.TypeMismatchException;
+import org.junit.Test;
+
+public class ArrayRemoveFunctionIT extends ParallelStatsDisabledIT {
+private String initTables(Connection conn) throws Exception {
+String tableName = generateUniqueName();
+String ddl = "CREATE TABLE " + tableName
++ " (region_name VARCHAR PRIMARY KEY,varchars
VARCHAR[],integers INTEGER[],doubles DOUBLE[],bigints BIGINT[],"
++ "chars CHAR(15)[],double1 DOUBLE,char1
CHAR(17),nullcheck INTEGER,chars2 CHAR(15)[], nullVarchar VARCHAR[], nullBigInt
BIGINT[],double2 DOUBLE,integer1 INTEGER)";
+conn.createStatement().execute(ddl);
+String dml = "UPSERT INTO " + tableName +
"(region_name,varchars,integers,doubles,bigints,chars,double1,char1,nullcheck,chars2,double2,integer1)
VALUES('SF Bay Area'," +
+"ARRAY['2345','46345','23234']," +
+"ARRAY[2345,46345,23234,456]," +
+"ARRAY[23.45,46.345,23.234,45.6,5.78]," +
+"ARRAY[12,34,56,78,910]," +
+"ARRAY['a','','c','ddd','e']," +
+"23.45," +
+"'wert'," +
+"NULL," +
+"ARRAY['a','','c','ddd','e','foo']," +
+"12,"+
+"12"+
+")";
+PreparedStatement stmt = conn.prepareStatement(dml);
+stmt.execute();
+conn.commit();
+return tableName;
+}
+
+@Test
+public void testEmptyArrayModification() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String tableName = initTables(conn);
+
+ResultSet rs;
+rs = conn.createStatement().executeQuery("SELECT
ARRAY_REMOVE(nullVarChar,'34567') FROM " + tableName + " LIMIT 1");
+assertTrue(rs.next());
+
+assertNull(rs.getArray(1));
+assertFalse(rs.next());
+}
+
+@Test
+public void testArrayRemoveFunctionVarchar() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String tableName = initTables(conn);
+
+ResultSet rs;
+rs = conn.createStatement().executeQuery("SELECT
ARRAY_REMOVE(varchars,'23234') FROM " + tableName + " WHERE region_name = 'SF
Bay Area'");
+assertTrue(rs.next());
+
+String[] strings = new String[]{"2345", "46345"};
+
+Array array = conn.createArrayOf("VARCHAR", strings);
+
+assertEquals(array, rs.getArray(1));
+assertFalse(rs.next());
+}
+
+@Test
+public void testArrayRemoveFunctionInteger() throws Exception {
+Connection conn = DriverManager.getConnection(getUrl());
+String t
[GitHub] phoenix pull request #294: PHOENIX-4643 Implement ARRAY_REMOVE built in func...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/294#discussion_r175226865
--- Diff:
phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayRemoveFunctionIT.java
---
@@ -0,0 +1,425 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import java.sql.Array;
+import java.sql.Connection;
+import java.sql.DriverManager;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+
+import org.apache.phoenix.schema.TypeMismatchException;
+import org.junit.Test;
+
+public class ArrayRemoveFunctionIT extends ParallelStatsDisabledIT {
+private String initTables(Connection conn) throws Exception {
+String tableName = generateUniqueName();
+String ddl = "CREATE TABLE " + tableName
++ " (region_name VARCHAR PRIMARY KEY,varchars
VARCHAR[],integers INTEGER[],doubles DOUBLE[],bigints BIGINT[],"
++ "chars CHAR(15)[],double1 DOUBLE,char1
CHAR(17),nullcheck INTEGER,chars2 CHAR(15)[], nullVarchar VARCHAR[], nullBigInt
BIGINT[],double2 DOUBLE,integer1 INTEGER)";
+conn.createStatement().execute(ddl);
+String dml = "UPSERT INTO " + tableName +
"(region_name,varchars,integers,doubles,bigints,chars,double1,char1,nullcheck,chars2,double2,integer1)
VALUES('SF Bay Area'," +
+"ARRAY['2345','46345','23234']," +
+"ARRAY[2345,46345,23234,456]," +
+"ARRAY[23.45,46.345,23.234,45.6,5.78]," +
+"ARRAY[12,34,56,78,910]," +
+"ARRAY['a','','c','ddd','e']," +
+"23.45," +
+"'wert'," +
+"NULL," +
+"ARRAY['a','','c','ddd','e','foo']," +
+"12,"+
+"12"+
+")";
+PreparedStatement stmt = conn.prepareStatement(dml);
+stmt.execute();
+conn.commit();
+return tableName;
+}
+
--- End diff --
Add tests for:
* removing first array element
* removing last array element
* removing middle array element
* removing only element in array
* removing element which repeats (what should happen?)
* removing CHAR(10) from VARCHAR
* removing VARCHAR from CHAR(10)
---
[GitHub] phoenix issue #294: PHOENIX-4643 Implement ARRAY_REMOVE built in function
Github user JamesRTaylor commented on the issue: https://github.com/apache/phoenix/pull/294 Changes look good. I've committed PHOENIX-4644, so can you make sure everything works without specify a default value for the second argument for ARRAY_REMOVE? ---
[GitHub] phoenix pull request #294: PHOENIX-4643 Implement ARRAY_REMOVE built in func...
Github user JamesRTaylor commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/294#discussion_r173612840
--- Diff:
phoenix-core/src/main/java/org/apache/phoenix/expression/function/ArrayRemoveFunction.java
---
@@ -0,0 +1,79 @@
+/*
+ * 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.expression.function;
+
+import java.util.List;
+
+import org.apache.hadoop.hbase.io.ImmutableBytesWritable;
+import org.apache.phoenix.expression.Expression;
+import org.apache.phoenix.parse.FunctionParseNode;
+import org.apache.phoenix.schema.SortOrder;
+import org.apache.phoenix.schema.TypeMismatchException;
+import org.apache.phoenix.schema.types.PArrayDataTypeDecoder;
+import org.apache.phoenix.schema.types.PArrayDataTypeEncoder;
+import org.apache.phoenix.schema.types.PBinaryArray;
+import org.apache.phoenix.schema.types.PDataType;
+import org.apache.phoenix.schema.types.PVarbinary;
+import org.apache.phoenix.schema.types.PVarbinaryArray;
+
+@FunctionParseNode.BuiltInFunction(name = ArrayRemoveFunction.NAME, args =
{
+ @FunctionParseNode.Argument(allowedTypes = {
PBinaryArray.class, PVarbinaryArray.class }),
+ @FunctionParseNode.Argument(allowedTypes = { PVarbinary.class
}, defaultValue = "null") })
+public class ArrayRemoveFunction extends ArrayModifierFunction {
+
+ public static final String NAME = "ARRAY_REMOVE";
+
+ public ArrayRemoveFunction() {
+ }
+
+ public ArrayRemoveFunction(List children) throws
TypeMismatchException {
+ super(children);
+ }
+
+ @Override
+ protected boolean modifierFunction(ImmutableBytesWritable ptr, int
length, int offset, byte[] arrayBytes,
+ PDataType baseType, int arrayLength, Integer maxLength,
Expression arrayExp) {
+ SortOrder sortOrder = arrayExp.getSortOrder();
+
+ if (ptr.getLength() == 0 || arrayBytes.length == 0) {
+ ptr.set(arrayBytes, offset, length);
+ return true;
+ }
+
+ PArrayDataTypeEncoder arrayDataTypeEncoder = new
PArrayDataTypeEncoder(baseType, sortOrder);
+
+ for (int arrayIndex = 0; arrayIndex < arrayLength;
arrayIndex++) {
+ ImmutableBytesWritable ptr2 = new
ImmutableBytesWritable(arrayBytes, offset, length);
+ PArrayDataTypeDecoder.positionAtArrayElement(ptr2,
arrayIndex, baseType, maxLength);
+ if (baseType.compareTo(ptr2, sortOrder, ptr, sortOrder,
baseType) != 0) {
--- End diff --
This looks good. Can you make sure you have tests around removing an
element from an array where the element type is slightly different than the
array element type? For example, removing an int from a long array or removing
a long from a decimal array?
---
