-
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-6649 TransformTool to support views and tenant views #1397
Conversation
2fd8eec
to
ec9d1ce
Compare
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, just a request for one extra test please
} | ||
PTable newView = Transform.getTransformedView(decendant, newDataTable, columnMap, true); | ||
QueryPlan queryPlan = getQueryPlan(newView, decendant, connection); | ||
inputSplits.addAll(generateSplits(queryPlan, configuration)); |
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 the use of the setMultiInputMapperSplitSize in the TransformTool mean that multiple splits can be worked by the same mapper? (If so that's very good, because we can have lots of small views.) Either way, there's probably lots of future optimization work based on the view size, but we need better stats to be able to do that sort of thing so we can defer for now.
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.
Let me add a parameter to transform tool since it is 10 now (default).
PTable newView = Transform.getTransformedView(decendant, newDataTable, columnMap, true); | ||
QueryPlan queryPlan = getQueryPlan(newView, decendant, connection); | ||
inputSplits.addAll(generateSplits(queryPlan, configuration)); | ||
} |
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 happens if the views are not disjoint? Do we just harmlessly transform the same data multiple times into the same shape? Seems like we should have a test for that if we don't already.
} | ||
|
||
|
||
public static Connection getTenantConnection(String tenant) throws SQLException { |
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.
Could we please get a test for overlapping but still updatable views? (For example, one view is WHERE Foo = 'a' AND Bar = 'b', and another is just WHERE Foo = 'a'.)
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.
In TransformMonitorExtendedIT I believe you have an updatable and non-updatable tenant view that overlap, but since we skip transforming read-only views, that's not quite the same thing.
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.
Sure. Will add one.
ec9d1ce
to
e45d386
Compare
stmt1.executeUpdate(); | ||
|
||
createViewSql = "CREATE VIEW " + view2Name + " ( VIEW_COL1 INTEGER, VIEW_COL2 VARCHAR ) AS SELECT * FROM " | ||
+ dataTableFullName1 + " where DATA='GLOBAL_VIEW' AND ZIP=95053"; |
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.
@gjacoby126 for overlapping 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.
+1, thanks @gokceni
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.
Never mind, PhoenixTransformWithViewsInputFormat looks like it's missing an Apache License. Could you please fix before committing?
e45d386
to
f78b910
Compare
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
Merged |
No description provided.