gjacoby126 commented on a change in pull request #1170:
URL: https://github.com/apache/phoenix/pull/1170#discussion_r605997136
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java
##########
@@ -728,6 +729,13 @@ 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:
why _IDX_ + logical name of base table for view index? That's the
physical table the view index is stored in, not the logical name.
##########
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 getPhysicalTableNameColumnInSyscat();
Review comment:
Thinking about this more, I don't think just changing the name solves
the problem of having two methods that are almost-but-not-quite the same thing.
I'd still be confused about which to call.
Can we not use the existing getPhysicalNames() to return the physical table
name column from syscat in the situations where that's appropriate?
##########
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:
Since we generate proto code on-demand now, I don't think we gain
anything by clumping unrelated protobuf changes into this PR. We can save
isModifiable for next time when the change can be considered as a whole
##########
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 getPhysicalTableNameColumnInSyscat();
Review comment:
getPhysicalNames makes a point of returning a list of PNames to leave
the interface open for views that span multiple tables (not currently supported
but long on the wishlist). Does anything in this PR, such as
getPhysicalTableNameColumnInSyscat returning a single String, prevent us from
having multi-table views later?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/schema/PTable.java
##########
@@ -728,6 +729,13 @@ 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
+ * Ex: For hierarchical views like tableLogicalName --> view1 --> view2,
for view2, returns _IDX_+tableLogicalName
+ */
+ PName getParentLogicalName();
Review comment:
Likewise with getParentName vs getParentLogicalName. If I'm reading the
current logic right, getParentName _also_ returns only logical names. They're
almost-but-not-quite the same thing. Can we consolidate? Otherwise we're going
to create lots of subtle, really-hard-to-spot bugs going forward when people
use the wrong one.
--
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]
