New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6271: Effective DDL generated by SchemaExtractionTool should … #1212
Conversation
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some nits
@@ -1275,15 +1275,21 @@ public static String getPTableFullNameWithQuotes(String pSchemaName, String pTab | |||
pTableName = "\""+pTableName+"\""; | |||
} | |||
if(tableNameNeedsQuotes || schemaNameNeedsQuotes) { | |||
pTableFullName = pSchemaName + "." + pTableName; | |||
if (pSchemaName != null && !pSchemaName.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Strings.isNullOrEmpty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do you need to think about namespaceenabled? I am guessing no
return pTableFullName; | ||
} | ||
|
||
private static boolean isQuotesNeeded(String name) { | ||
// first char numeric or non-underscore | ||
if(!Character.isAlphabetic(name.charAt(0)) && name.charAt(0)!='_') { | ||
if (name == null || name.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Strings.isNullOrEmpty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misunderstanding what you're trying to do in this patch, but the results for indexes look incorrect to me. The effective DDL of an index shouldn't contain the PK suffix inherited from the base table or parent view. (I can see it being useful to include such a thing in a SQL comment or something, but not the CREATE INDEX statement itself)
if (effectivePK.isEmpty()) { | ||
effectivePK = indexPkSet; | ||
// This is added because of PHOENIX-2340 | ||
if (dataPTable.isMultiTenant() && indexPKName.contains(dataPKName.get(0))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we have a String tenantId = dataPKName.get(0); line and use that in the checks? I worry that if we ever implement PHOENIX-5248 that the current logic here will be too subtle for the implementer to realize that it needs to be changed if we can't depend on first column == tenantId.
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Outdated
Show resolved
Hide resolved
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Show resolved
Hide resolved
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Show resolved
Hide resolved
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Outdated
Show resolved
Hide resolved
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Outdated
Show resolved
Hide resolved
…maintain the order of PK and other columns
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, thanks for the patch and adding the extra test assertions, @swaroopak
@@ -47,6 +47,7 @@ | |||
|
|||
import javax.annotation.Nullable; | |||
|
|||
import com.google.common.base.Strings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember that you'll have to use the shaded version when you port to master.
phoenix-tools/src/it/java/org/apache/phoenix/schema/SchemaExtractionToolIT.java
Show resolved
Hide resolved
Thank you for the review @gjacoby126 @gokceni |
…maintain the order of PK and other columns (#1212) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…maintain the order of PK and other columns (apache#1212) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…maintain the order of PK and other columns (#1212) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…maintain the order of PK and other columns