Uh oh!
There was an error while loading. Please reload this page.
PHOENIX-6247 Separating logical and physical table names - #1170
Closed
gokceni wants to merge 1 commit into
Closed
Conversation
stoty
commented
Mar 18, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
gokceniforce-pushed
the
separatePhysical
branch
2 times, most recently
from
March 19, 2021 20:51
6af705e to
ff6b4a1Comparestoty
commented
Mar 20, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 21, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
left a comment
Contributor
There was a problem hiding this comment.
Initial comments -- reviewed about 50% so far.
| assertEquals(0, exitCode); | ||
| ResultSet rs = stmt.executeQuery("SELECT * FROM " + tableName); | ||
| assertFalse(rs.next()); |
Contributor
There was a problem hiding this comment.
I don't understand this check -- how can SELECT * FROM FOO return no rows but SELECT * FROM FOO WHERE return a row?
ContributorAuthor
There was a problem hiding this comment.
The import tool is actually importing on the index table without touching the data table, that is why data table is empty but index table is not empty. I think it will make more sense if I remove --index-table param and import it on the data table and check that the data table whose physical name is changed got populated. And then to check index, change the physical index table name too and then use --index-table param.
Contributor
There was a problem hiding this comment.
This also assume that the Phoenix query planner chooses a specific plan. Better to hint the query with /*+ NO_INDEX */ if you do not want the index to be used.
Uh oh!
There was an error while loading. Please reload this page.
| props.put(QueryServices.MUTATE_BATCH_SIZE_ATTRIB, Integer.toString(3000)); | ||
| //When we run all tests together we are using global cluster(driver) | ||
| //so to make drop work we need to re register driver with DROP_METADATA_ATTRIB property | ||
| destroyDriver(); |
Contributor
There was a problem hiding this comment.
Rather than doing this manually, should this be a NeedsOwnCluster test?
Uh oh!
There was an error while loading. Please reload this page.
| createIndexOnTable(conn, fullTableName, indexName); | ||
| } | ||
| SingleCellIndexIT.dumpTable(fullNewTableName); |
Contributor
There was a problem hiding this comment.
Consider using TestUtil.dumpTable. Also, do we need to output to stdout in the final version of this? Can probably be cut.
ContributorAuthor
There was a problem hiding this comment.
Yeah, we don't need it in the end. This one is simpler to call since it just takes the string tablename. The other one requires HTableInterface.
Contributor
There was a problem hiding this comment.
Before merging could you please remove?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
swaroopak
left a comment
Contributor
There was a problem hiding this comment.
Initial review. Since started some time back, ignore the duplicates with Geoffrey's if any :)
Uh oh!
There was an error while loading. Please reload this page.
| // 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); |
Contributor
There was a problem hiding this comment.
I think the comment above won't be relevant anymore?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| SEPARATOR_BYTE_ARRAY, Bytes.toBytes(familyName)); | ||
| } | ||
| public static PName getTableName(PName schemaName, PName tableName) { |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
stoty
commented
Mar 22, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 23, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
gokceniforce-pushed
the
separatePhysical
branch
2 times, most recently
from
March 24, 2021 23:06
f49ab4c to
32312f3Comparestoty
commented
Mar 25, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 25, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 26, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Mar 26, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
swaroopak
left a comment
Contributor
There was a problem hiding this comment.
Minor comments, rest LGTM.
Uh oh!
There was an error while loading. Please reload this page.
| assertEquals(cnt, expected.size()); | ||
| } | ||
| public static void renameAndDropPhysicalTable(Connection conn, String tenantId, String schema, String tableName, String physicalName) throws Exception { |
Contributor
There was a problem hiding this comment.
should this be broken into 2 and call them "assignNewPhysicalTable" and "dropOldPhysicalTable"?
ContributorAuthor
There was a problem hiding this comment.
I don't have a case right now that requires separate methods. It is a good test to do the switch and drop so that we are sure the old table is not used. In the future, I will consider separating.
Uh oh!
There was an error while loading. Please reload this page.
| if (parentTable == null) { | ||
| physicalTables.add(famName); | ||
| // If this is a view index, then one of the link is IDX_VW -> _IDX_ PhysicalTable link. Since famName is _IDX_ and we can't get this table hence it is null, we need to use actual view name | ||
| parentLogicalName = (tableType == INDEX ? SchemaUtil.getTableName(parentSchemaName, parentTableName) : famName); |
Contributor
There was a problem hiding this comment.
somthing like INDEX.equalsIgnoreCase(tableType) instad of ==
ContributorAuthor
There was a problem hiding this comment.
INDEX is PTableType not string
gjacoby126
commented
Mar 31, 2021
Contributor
@ChinmaySKulkarni - this is a significant revision of how Phoenix approaches metadata naming by @gokceni , would appreciate your opinion as well. |
gjacoby126
requested changes
Mar 31, 2021
| 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(); |
Contributor
There was a problem hiding this 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.
ContributorAuthor
There was a problem hiding this comment.
There was only one place calling this function and it was using parentTable.getPhysicalName(); as physicalName. That mapped to full name of base table like SC1.TBL_1. getParentLogicalName returns the same now. But parentTable.getPhysicalName returns SC1.NEW_PHYSICALNAME_TBL1 which we don't.
I run all view related IT tests. Is there any other tests you would like me to run to check for collusions?
ContributorAuthor
There was a problem hiding this comment.
Basically if you change the physical name of a table, this function is going to use the logical name instead of physical name of the base table. So the sequence will not go back to the beginning. In the next pr, I will create a view index, rename the table and create another view index to see that it is not colliding. This should be enough right?
| PName getName(); | ||
| PName getSchemaName(); | ||
| PName getTableName(); | ||
| PName getPhysicalTableName(); |
Contributor
There was a problem hiding this comment.
Distinction between getPhysicalTableName and getPhysicalName can be confusing
ContributorAuthor
There was a problem hiding this comment.
Renamed getPhysicalTableName as getPhysicalTableNameColumnInSyscat. getPhysicalName is inferred and I could have renamed it but that would touch a lot of files.
| 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 |
Contributor
There was a problem hiding this comment.
Why the parent base table and not the immediate parent?
ContributorAuthor
There was a problem hiding this comment.
This is the existing logic. If this is a child view of another view, the parent is the immediate parent view. If it is a view directly on top of a table, the parent is the logical name of the table. Removed that comment.
| 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 |
Contributor
There was a problem hiding this comment.
nit: remove "update to above comment"
| } | ||
| @Override | ||
| public final PName getPhysicalTableName() { |
Contributor
There was a problem hiding this comment.
As mentioned elsewhere, why both the existing getPhysicalName and a new getPhysicalTableName? Can they be consolidated? Or at least named something more clear?
ContributorAuthor
There was a problem hiding this comment.
It is a bit complicated. getPhysicalName is mostly used for views. getPhysicalTableName is mostly for non-views and maps to a column on syscat. Let me see what I can do.
| @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. |
Contributor
There was a problem hiding this comment.
For views, physicalName is the base table physical table name or logical table name?
ContributorAuthor
There was a problem hiding this comment.
physical. Will update the comment
| } | ||
| public static String getViewIndexSequenceSchemaName(PName physicalName, boolean isNamespaceMapped) { | ||
| public static String getViewIndexSequenceSchemaName(PName logicalName, boolean isNamespaceMapped) { |
Contributor
There was a problem hiding this 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.
ContributorAuthor
There was a problem hiding this comment.
Will change to logicalBaseTableName
| optional int64 lastDDLTimestamp = 45; | ||
| optional bool changeDetectionEnabled = 46; | ||
| optional bytes physicalTableNameBytes = 47; | ||
| optional bool isModifiable = 48; |
ContributorAuthor
There was a problem hiding this comment.
It will be set later as part of another change. Since I was changing this part, I added this one as well
Contributor
There was a problem hiding this 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
stoty
commented
Apr 1, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Apr 1, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
requested changes
Apr 1, 2021
gjacoby126
left a comment
Contributor
There was a problem hiding this comment.
I think the new methods here, and what the metadata methods on PTable around naming should be going forward, needs more thought. The current PR proposes two new PTable properties that are almost-but-not-quite the same as existing properties.
It's really important to get the interfaces right because they'll be hard to change later. Maybe a design doc on the JIRA is a better place for that conversation than a PR?
@ChinmaySKulkarni fyi.
| optional int64 lastDDLTimestamp = 45; | ||
| optional bool changeDetectionEnabled = 46; | ||
| optional bytes physicalTableNameBytes = 47; | ||
| optional bool isModifiable = 48; |
Contributor
There was a problem hiding this 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
| PName getName(); | ||
| PName getSchemaName(); | ||
| PName getTableName(); | ||
| PName getPhysicalTableNameColumnInSyscat(); |
Contributor
There was a problem hiding this 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?
ContributorAuthor
There was a problem hiding this comment.
getPhysicalName already returns physical table name column from syscat when appropriate.
We still need 2 methods. One represents the actual column in Syscat. The other is inferred (like views or view indexes).
| PName getParentTableName(); | ||
| /** | ||
| * @return the logical name of the parent. In case of the view index, it is the _IDX_+logical name of base table |
Contributor
There was a problem hiding this 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.
ContributorAuthor
There was a problem hiding this comment.
I am not sure I understand your comment. IDX prefix + logical name of the table is where the view index is stored in.
| * @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(); |
Contributor
There was a problem hiding this 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.
| PName getName(); | ||
| PName getSchemaName(); | ||
| PName getTableName(); | ||
| PName getPhysicalTableNameColumnInSyscat(); |
Contributor
There was a problem hiding this 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?
stoty
commented
Apr 5, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
stoty
commented
Apr 6, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
gjacoby126
requested changes
Apr 7, 2021
gjacoby126
left a comment
Contributor
There was a problem hiding this comment.
Just some nits and then I think we're good to merge
Uh oh!
There was an error while loading. Please reload this page.
| MetaDataMutationResult parentResult = updateCache(connection.getTenantId(), parentSchemaName, parentTableName, | ||
| false, resolvedTimestamp); | ||
| PTable parentTable = parentResult.getTable(); | ||
| if (LOGGER.isDebugEnabled()) { |
Contributor
There was a problem hiding this comment.
Do we need this logging? Wondering if it should be at TRACE level to avoid having a bunch of logs of it, since I think this is a pretty frequently used function.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| createIndexOnTable(conn, fullTableName, indexName); | ||
| } | ||
| SingleCellIndexIT.dumpTable(fullNewTableName); |
Contributor
There was a problem hiding this comment.
Before merging could you please remove?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gokceni
commented
Apr 7, 2021
ContributorAuthor
Merged to 4.x-PHOENIX-6247 branch. Thanks for the review! |
stoty
commented
Apr 8, 2021
Contributor
💔 -1 overall
This message was automatically generated. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.