gjacoby126 commented on a change in pull request #1170:
URL: https://github.com/apache/phoenix/pull/1170#discussion_r605253952
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java
##########
@@ -583,6 +583,7 @@ private static int getReservedQualifier(byte[] bytes, int
offset, int length) {
PName getName();
PName getSchemaName();
PName getTableName();
+ PName getPhysicalTableName();
Review comment:
Distinction between getPhysicalTableName and getPhysicalName can be
confusing
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/coprocessor/MetaDataEndpointImpl.java
##########
@@ -2152,10 +2201,15 @@ public void createTable(RpcController controller,
CreateTableRequest request,
}
}
- private long getViewIndexSequenceValue(PhoenixConnection connection,
String tenantIdStr, PTable parentTable, PName physicalName) throws SQLException
{
+ private long getViewIndexSequenceValue(PhoenixConnection connection,
String tenantIdStr, PTable parentTable) throws SQLException {
int nSequenceSaltBuckets =
connection.getQueryServices().getSequenceSaltBuckets();
-
- SequenceKey key = MetaDataUtil.getViewIndexSequenceKey(tenantIdStr,
physicalName,
+ // parentTable is parent of the view index which is the view.
+ // Since parent is the view, the parentTable.getParentLogicalName()
returns the logical full name of the base table
+ PName parentName = parentTable.getParentLogicalName();
Review comment:
I'm a bit confused here. In the original logic, we passed in a physical
name, but now we pass in a logical name if parentTable (the view) has one
defined and a physical name if it doesn't. From the design doc you shared with
me, sounds like it should usually be a constant logical name?
It's actually really important that we use the same table name here in all
cases -- that all indexes on all views on a particular physical base table use
the same sequence to generate view index ids. Otherwise you can get collisions
between view index ids.
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1008,6 +1030,11 @@ public final PName getTableName() {
return tableName;
}
+ @Override
+ public final PName getPhysicalTableName() {
Review comment:
As mentioned elsewhere, why both the existing getPhysicalName and a new
getPhysicalTableName? Can they be consolidated? Or at least named something
more clear?
##########
File path: phoenix-core/src/main/protobuf/PTable.proto
##########
@@ -111,6 +111,9 @@ message PTable {
optional bool viewModifiedPhoenixTTL = 44;
optional int64 lastDDLTimestamp = 45;
optional bool changeDetectionEnabled = 46;
+ optional bytes physicalTableNameBytes = 47;
+ optional bool isModifiable = 48;
Review comment:
where is isModifiable set?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1559,7 +1586,29 @@ 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
Review comment:
Why the parent base table and not the immediate parent?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1586,7 +1635,12 @@ public synchronized boolean
getIndexMaintainers(ImmutableBytesWritable ptr, Phoe
@Override
public PName getPhysicalName() {
+ // For views, physicalName is base table name. There might be a case
where the Phoenix table is pointing to another physical table.
Review comment:
For views, physicalName is the base table _physical table name_ or
_logical_ table name?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTableImpl.java
##########
@@ -1559,7 +1586,29 @@ 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
Review comment:
nit: remove "update to above comment"
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java
##########
@@ -678,13 +691,13 @@ 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) {
Review comment:
should be logicalParentName or logicalBaseTableName to make it clear
that this is not the logical name of the view, but the suffix of the _IDX_
table so that all view indexes of the same base table get the same view index
sequence.
--
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]
