[
https://issues.apache.org/jira/browse/PHOENIX-6247?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17306405#comment-17306405
]
ASF GitHub Bot commented on PHOENIX-6247:
-----------------------------------------
swaroopak commented on a change in pull request #1170:
URL: https://github.com/apache/phoenix/pull/1170#discussion_r597846333
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/util/PhoenixRuntime.java
##########
@@ -44,6 +44,7 @@
import javax.annotation.Nullable;
+import org.apache.hadoop.hbase.util.Bytes;
Review comment:
nit: unused import?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java
##########
@@ -678,39 +678,39 @@ public static SequenceKey
getOldViewIndexSequenceKey(String tenantId, PName phys
return new SequenceKey(isNamespaceMapped ? tenantId : null,
schemaName, tableName, nSaltBuckets);
}
- public static String getViewIndexSequenceSchemaName(PName physicalName,
boolean isNamespaceMapped) {
+ public static String getViewIndexSequenceSchemaName(PName logicalName,
boolean isNamespaceMapped) {
if (!isNamespaceMapped) {
- String baseTableName =
SchemaUtil.getParentTableNameFromIndexTable(physicalName.getString(),
+ String baseTableName =
SchemaUtil.getParentTableNameFromIndexTable(logicalName.getString(),
MetaDataUtil.VIEW_INDEX_TABLE_PREFIX);
return SchemaUtil.getSchemaNameFromFullName(baseTableName);
} else {
- return
SchemaUtil.getSchemaNameFromFullName(physicalName.toString());
+ return
SchemaUtil.getSchemaNameFromFullName(logicalName.toString());
}
}
- public static String getViewIndexSequenceName(PName physicalName, PName
tenantId, boolean isNamespaceMapped) {
- return SchemaUtil.getTableNameFromFullName(physicalName.toString()) +
VIEW_INDEX_SEQUENCE_NAME_PREFIX;
+ public static String getViewIndexSequenceName(PName logicalName, PName
tenantId, boolean isNamespaceMapped) {
+ return SchemaUtil.getTableNameFromFullName(logicalName.toString()) +
VIEW_INDEX_SEQUENCE_NAME_PREFIX;
}
/**
*
* @param tenantId No longer used, but kept in signature for backwards
compatibility
- * @param physicalName Name of physical view index table
+ * @param logicalName Name of physical view index table
* @param nSaltBuckets Number of salt buckets
* @param isNamespaceMapped Is namespace mapping enabled
* @return SequenceKey for the ViewIndexId
*/
- public static SequenceKey getViewIndexSequenceKey(String tenantId, PName
physicalName, int nSaltBuckets,
+ public static SequenceKey getViewIndexSequenceKey(String tenantId, PName
logicalName, int nSaltBuckets,
boolean isNamespaceMapped) {
// Create global sequence of the form: <prefixed base table name>.
// We can't use a tenant-owned or escaped sequence because of
collisions,
// with other view indexes that may be global or owned by other
tenants that
// also use this same physical view index table. It's also much easier
// to cleanup when the physical table is dropped, as we can delete
// all global sequences leading with <prefix> + physical name.
- String schemaName = getViewIndexSequenceSchemaName(physicalName,
isNamespaceMapped);
Review comment:
I think the comment above won't be relevant anymore?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1851,112 +1905,126 @@ public static PTable
createFromProto(PTableProtos.PTable table) {
}
public static PTableProtos.PTable toProto(PTable table) {
- PTableProtos.PTable.Builder builder = PTableProtos.PTable.newBuilder();
- if(table.getTenantId() != null){
- builder.setTenantId(ByteStringer.wrap(table.getTenantId().getBytes()));
- }
-
builder.setSchemaNameBytes(ByteStringer.wrap(table.getSchemaName().getBytes()));
-
builder.setTableNameBytes(ByteStringer.wrap(table.getTableName().getBytes()));
- builder.setTableType(ProtobufUtil.toPTableTypeProto(table.getType()));
- if (table.getType() == PTableType.INDEX) {
- if(table.getIndexState() != null) {
- builder.setIndexState(table.getIndexState().getSerializedValue());
- }
- if(table.getViewIndexId() != null) {
- builder.setViewIndexId(table.getViewIndexId());
- builder.setViewIndexIdType(table.getviewIndexIdType().getSqlType());
- }
- if(table.getIndexType() != null) {
- builder.setIndexType(ByteStringer.wrap(new
byte[]{table.getIndexType().getSerializedValue()}));
- }
- }
- builder.setSequenceNumber(table.getSequenceNumber());
- builder.setTimeStamp(table.getTimeStamp());
- PName tmp = table.getPKName();
- if (tmp != null) {
- builder.setPkNameBytes(ByteStringer.wrap(tmp.getBytes()));
- }
- Integer bucketNum = table.getBucketNum();
- int offset = 0;
- if(bucketNum == null){
- builder.setBucketNum(NO_SALTING);
- } else {
- offset = 1;
- builder.setBucketNum(bucketNum);
- }
- List<PColumn> columns = table.getColumns();
- int columnSize = columns.size();
- for (int i = offset; i < columnSize; i++) {
- PColumn column = columns.get(i);
- builder.addColumns(PColumnImpl.toProto(column));
- }
- List<PTable> indexes = table.getIndexes();
- for (PTable curIndex : indexes) {
- builder.addIndexes(toProto(curIndex));
- }
- builder.setIsImmutableRows(table.isImmutableRows());
- // TODO remove this field in 5.0 release
- if (table.getParentName() != null) {
-
builder.setDataTableNameBytes(ByteStringer.wrap(table.getParentTableName().getBytes()));
- }
- if (table.getParentName() !=null) {
-
builder.setParentNameBytes(ByteStringer.wrap(table.getParentName().getBytes()));
- }
- if (table.getDefaultFamilyName()!= null) {
-
builder.setDefaultFamilyName(ByteStringer.wrap(table.getDefaultFamilyName().getBytes()));
- }
- builder.setDisableWAL(table.isWALDisabled());
- builder.setMultiTenant(table.isMultiTenant());
- builder.setStoreNulls(table.getStoreNulls());
- if (table.getTransactionProvider() != null) {
-
builder.setTransactionProvider(table.getTransactionProvider().getCode());
- }
- if(table.getType() == PTableType.VIEW){
- builder.setViewType(ByteStringer.wrap(new
byte[]{table.getViewType().getSerializedValue()}));
- }
- if(table.getViewStatement()!=null){
-
builder.setViewStatement(ByteStringer.wrap(PVarchar.INSTANCE.toBytes(table.getViewStatement())));
- }
- for (int i = 0; i < table.getPhysicalNames().size(); i++) {
-
builder.addPhysicalNames(ByteStringer.wrap(table.getPhysicalNames().get(i).getBytes()));
- }
- builder.setBaseColumnCount(table.getBaseColumnCount());
- builder.setRowKeyOrderOptimizable(table.rowKeyOrderOptimizable());
- builder.setUpdateCacheFrequency(table.getUpdateCacheFrequency());
- builder.setIndexDisableTimestamp(table.getIndexDisableTimestamp());
- builder.setIsNamespaceMapped(table.isNamespaceMapped());
- if (table.getAutoPartitionSeqName() != null) {
- builder.setAutoParititonSeqName(table.getAutoPartitionSeqName());
- }
- builder.setIsAppendOnlySchema(table.isAppendOnlySchema());
- if (table.getImmutableStorageScheme() != null) {
- builder.setStorageScheme(ByteStringer.wrap(new
byte[]{table.getImmutableStorageScheme().getSerializedMetadataValue()}));
- }
- if (table.getEncodedCQCounter() != null) {
- Map<String, Integer> values = table.getEncodedCQCounter().values();
- for (Entry<String, Integer> cqCounter : values.entrySet()) {
-
org.apache.phoenix.coprocessor.generated.PTableProtos.EncodedCQCounter.Builder
cqBuilder =
org.apache.phoenix.coprocessor.generated.PTableProtos.EncodedCQCounter.newBuilder();
- cqBuilder.setColFamily(cqCounter.getKey());
- cqBuilder.setCounter(cqCounter.getValue());
- builder.addEncodedCQCounters(cqBuilder.build());
- }
- }
- if (table.getEncodingScheme() != null) {
- builder.setEncodingScheme(ByteStringer.wrap(new
byte[]{table.getEncodingScheme().getSerializedMetadataValue()}));
- }
- if (table.useStatsForParallelization() != null) {
-
builder.setUseStatsForParallelization(table.useStatsForParallelization());
- }
- builder.setPhoenixTTL(table.getPhoenixTTL());
- builder.setPhoenixTTLHighWaterMark(table.getPhoenixTTLHighWaterMark());
-
builder.setViewModifiedUpdateCacheFrequency(table.hasViewModifiedUpdateCacheFrequency());
-
builder.setViewModifiedUseStatsForParallelization(table.hasViewModifiedUseStatsForParallelization());
- builder.setViewModifiedPhoenixTTL(table.hasViewModifiedPhoenixTTL());
- if (table.getLastDDLTimestamp() != null) {
- builder.setLastDDLTimestamp(table.getLastDDLTimestamp());
- }
- builder.setChangeDetectionEnabled(table.isChangeDetectionEnabled());
- return builder.build();
+ PTableProtos.PTable.Builder builder = PTableProtos.PTable.newBuilder();
Review comment:
Is it only indentation change?
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/mapreduce/index/IndexScrutinyMapper.java
##########
@@ -343,18 +343,17 @@ private int getTableTtl() throws SQLException,
IOException {
@VisibleForTesting
public static String getSourceTableName(PTable pSourceTable, boolean
isNamespaceEnabled) {
- String sourcePhysicalName = pSourceTable.getPhysicalName().getString();
Review comment:
pSourceTable.getPhysicalName().getString() can still be used as a
variable since being called at multiple places.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/iterate/BaseResultIterators.java
##########
@@ -105,6 +105,7 @@
import org.apache.phoenix.schema.ColumnFamilyNotFoundException;
import org.apache.phoenix.schema.PColumn;
import org.apache.phoenix.schema.PColumnFamily;
+import org.apache.phoenix.schema.PNameFactory;
Review comment:
nit: unused import?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1559,7 +1588,17 @@ public PName getParentTableName() {
@Override
public PName getParentName() {
// a view on a table will not have a parent name but will have a
physical table name (which is the parent)
- return (type!=PTableType.VIEW || parentName!=null) ? parentName :
getPhysicalName();
+ // Update to above comment: we will return logical name of view parent
base table
+ return (type!=PTableType.VIEW || parentName!=null) ? parentName :
(parentLogicalName != null? parentLogicalName : getPhysicalName());
+ }
+
+ @Override
+ public PName getParentLogicalName() {
Review comment:
curious, what does it return for a table?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1583,10 +1622,15 @@ public synchronized boolean
getIndexMaintainers(ImmutableBytesWritable ptr, Phoe
ptr.set(indexMaintainersPtr.get(), indexMaintainersPtr.getOffset(),
indexMaintainersPtr.getLength());
return indexMaintainersPtr.getLength() > 0;
}
-
+ private static final Logger LOGGER =
LoggerFactory.getLogger(PTableImpl.class);
Review comment:
nit: this should be at the beginning of the class.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java
##########
@@ -728,6 +729,12 @@ private static int getReservedQualifier(byte[] bytes, int
offset, int length) {
* (use @getPhysicalTableName for this case)
*/
PName getParentTableName();
+
+ /**
+ * @return the logical name of the parent. In case of the view index, it
is the _IDX_+logical name of base table
Review comment:
comment for hierarchical views would be helpful as well.
Something like table --> view1 --> view2 what would be the parent logical
name in each case.
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/schema/PMetaDataImpl.java
##########
@@ -147,8 +157,14 @@ public void addTable(PTable table, long resolvedTime)
throws SQLException {
for (PTable index : table.getIndexes()) {
metaData.put(index.getKey(), tableRefFactory.makePTableRef(index,
this.timeKeeper.getCurrentTime(), resolvedTime));
}
+ if (table.getPhysicalTableName() != null &&
!table.getPhysicalTableName().getString().equals(table.getTableName().getString()))
{
+ String physicalTableName =
table.getPhysicalTableName().getString().replace(
+ QueryConstants.NAMESPACE_SEPARATOR,
QueryConstants.NAME_SEPARATOR);
+ String physicalTableFullName =
SchemaUtil.getTableName(table.getSchemaName() != null ?
table.getSchemaName().getString() : null, physicalTableName);
+ this.physicalNameToLogicalTableMap.put(physicalTableFullName, key);
+ }
}
-
+ private static final Logger LOGGER =
LoggerFactory.getLogger(PTableImpl.class);
Review comment:
nit: this should be at the beginning of the class
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/SchemaUtil.java
##########
@@ -328,6 +328,10 @@ private static boolean
isExistingTableMappedToPhoenixName(String name) {
SEPARATOR_BYTE_ARRAY, Bytes.toBytes(familyName));
}
+ public static PName getTableName(PName schemaName, PName tableName) {
Review comment:
nit: getTablePName?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/UpgradeUtil.java
##########
@@ -1331,7 +1331,7 @@ private static void
syncGlobalIndexesForTable(ConnectionQueryServices cqs, PTabl
*/
private static void syncViewIndexTable(ConnectionQueryServices cqs, PTable
baseTable, HColumnDescriptor defaultColFam,
Map<String, Object> syncedProps, Set<HTableDescriptor>
tableDescsToSync) throws SQLException {
- String viewIndexName =
MetaDataUtil.getViewIndexPhysicalName(baseTable.getPhysicalName().getString());
+ String viewIndexName =
MetaDataUtil.getViewIndexPhysicalName(baseTable.getName().getString());
Review comment:
curious, what does getName() return?
##########
File path:
phoenix-core/src/it/java/org/apache/phoenix/end2end/LogicalTableNameIT.java
##########
@@ -0,0 +1,793 @@
+package org.apache.phoenix.end2end;
+
+import com.google.common.base.Joiner;
+import com.google.common.collect.Lists;
+import com.google.common.collect.Maps;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.HBaseAdmin;
+import org.apache.hadoop.hbase.client.HTableInterface;
+import org.apache.hadoop.hbase.client.Put;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.mapreduce.Counters;
+import org.apache.hadoop.mapreduce.Job;
+import org.apache.phoenix.end2end.index.SingleCellIndexIT;
+import org.apache.phoenix.hbase.index.IndexRegionObserver;
+import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixDriver;
+import org.apache.phoenix.mapreduce.index.IndexScrutinyTool;
+import org.apache.phoenix.query.PhoenixTestBuilder;
+import org.apache.phoenix.query.QueryConstants;
+import org.apache.phoenix.query.QueryServices;
+import org.apache.phoenix.schema.PTable;
+import org.apache.phoenix.schema.types.PInteger;
+import org.apache.phoenix.util.*;
Review comment:
nit: avoid importing *
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> Change SYSTEM.CATALOG to allow separation of physical name (Hbase name) from
> logical name (Phoenix name)
> --------------------------------------------------------------------------------------------------------
>
> Key: PHOENIX-6247
> URL: https://issues.apache.org/jira/browse/PHOENIX-6247
> Project: Phoenix
> Issue Type: Improvement
> Reporter: Gokcen Iskender
> Assignee: Gokcen Iskender
> Priority: Major
>
> Currently, the tables in Phoenix have the same name as the underlying Hbase
> table. Separating logical and physical table name, ie. Having a Phoenix table
> point to an Hbase table with a different name have some advantages.
> An example is this: Let's say we want to have a different storage/encoding
> scheme for an index. We can build the new index while the clients use the old
> index and once the index is rebuilt, we can momentarily start pointing to the
> new index table without much downtime or performance implications. For the
> client, they are using the same index with the same name, but the physical
> table is different. Today, in order to change the index like this, we have to
> drop it and re-create which is a downtime for the index and the data table
> full scans are used for queries impacting performance while the index
> creation goes on.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
