-
Notifications
You must be signed in to change notification settings - Fork 994
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-6454: Add feature to SchemaTool to get the DDL in specificati… #1217
Conversation
💔 -1 overall
This message was automatically generated. |
|
||
@Test | ||
public void testCreateTableStatement_addColumn() throws Exception { | ||
String expected = "CREATE TABLE IF NOT EXISTS TEST.SAMPLE_TABLE" |
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: each of these would be easier to read if there was a comment saying what the difference was, such as "Adds RELATED_COMMAND" in this test
*/ | ||
package org.apache.phoenix.schema; | ||
|
||
public class SchemaProcessor { |
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.
Why a class and not an interface?
getCreateIndexStatement((CreateIndexStatement) createStatement, alterStatement); | ||
return getCreateIndexSQL(newCreateIndexStmt); | ||
} | ||
return ""; |
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.
What about CreateSequenceStatement and CreateFunctionStatement? And if none of the above, shouldn't we get an error of some kind?
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.
@swaroopak, This is a great start. As @gjacoby126 pointed out, there are more cases to cover. I think we need a design doc on this to explain how the design covers all the cases and explain how this tool will be used. I also think we can add add a future to Phoenix to self verify/test DDL statements. The place to add such a feature would be MetaDataClient. After every DDL, we can verify if DDL is correctly implemented by Phoenix.
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.
Thank you @kadirozde.
I started the doc per your suggestion. Meanwhile, do you think if this PR can be reviewed/merged for the initial scenarios (would be easier on reviews and incremental addition of code)? Or do you recommend not to merge until all of that is implemented? Please let me know.
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.
As far as I understand, this PR significantly refactors an existing tool. However, it does not cover some use cases. If so, refactoring should not reduce the functionality. So, I suggest a quick design review and a complete refactoring.
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.
This PR adds a new feature in SchemaTool with SYNTH mode. I added the design doc in the Jira with the current implementation and future extensions.
As per offline discussions, I'll change the input format from 2 files to 1 file. Thanks, @kadirozde
phoenix-tools/src/main/java/org/apache/phoenix/schema/SchemaSynthesisProcessor.java
Show resolved
Hide resolved
return newCreateIndexStmt; | ||
} | ||
|
||
private CreateTableStatement getCreateTableStatement(DropColumnStatement alterStatement, |
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: up to now we've been doing parameters as Create, Alter, and this switches to Alter, Create. Good to be consistent.
finalProps = | ||
ArrayListMultimap.<String, Pair<String, Object>>create(); | ||
for (Map.Entry<String, Object> entry : oldPropMap.entrySet()) { | ||
finalProps.put("", Pair.newPair(entry.getKey(), entry.getValue())); |
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.
What's the purpose of a hard-coded ["" -> [key -> value]] instead of just [key -> value]? Can't be duplicates, because oldPropMap was already a multi-map, right?
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.
Property multimap is formed with String and Pair as a key-value. Key for all properties pair is same "" but the value(pair of property key, value) is different. I had to stick to this to create a new CreateTableStatement. Not sure if I misunderstood your suggestion. LMK.
return finalProps; | ||
} | ||
|
||
private String getCreateTableSQL(CreateTableStatement createStmt) { |
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.
Good to extract these SQL-building helper functions into their own utility class that can be composed / injected. (For one thing, we're probably going to be adding the ability to output Statements to Avro schemas instead of SQL really soon now over at PHOENIX-6227)
💔 -1 overall
This message was automatically generated. |
ee2a953
to
d046736
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
4a11cfc
to
090d37e
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
d5c93bd
to
d6a924b
Compare
@gjacoby126 @kadirozde thank you for the review. I updated the PR please take a look. Thanks. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
@swaroopak - phoenix-tools unit tests are failing because of test coverage: and there's an ASF license warning for SchemaSQLUtil.java |
@@ -0,0 +1,151 @@ | |||
package org.apache.phoenix.schema; |
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.
Missing Apache license
.append("\nCONSTRAINT "+createStmt.getPrimaryKeyConstraint().getName()+" PRIMARY KEY") | ||
.append(" ("+createStmt.getPrimaryKeyConstraint().toString()+"))" | ||
.replaceAll(",", ",\n")); | ||
if(createStmt.getTableType().equals(PTableType.VIEW)) { |
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: space between if and (
import java.util.List; | ||
import java.util.Map; | ||
|
||
public class SchemaSQLUtil { |
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.
Does this utility class belong in tools? (that is, is it only useful to a particular tool or tools?)
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.
I don't know where else it would be used at this time. Would be happy to move it else where as need arises.
@gjacoby126 updated. Also, checked locally that the new IT class has 98% coverage of the class. |
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.
Looks good to me. Please open one or more Jira (if you have not done yet) to cover the remaining DDL statements.
💔 -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 changes, @swaroopak
…on mode (apache#1217) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…on mode (apache#1217) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…on mode (#1217) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…on mode (apache#1217) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…on mode (#1217) Co-authored-by: Swaroopa Kadam <s.kadam@apache.org>
…on mode