Skip to content
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-4855 Continue to write base table column metadata when creati… #339

Closed
wants to merge 1 commit into from

Conversation

twdsilva
Copy link
Contributor

…ng a view in order to support rollback

Previously in MetadataEndpointImpl we were removing parent column metadata while creating a child view. We still need to write the parent columns in order to be able to rollback if needed. This PR filters out the parent columns while creating the PTable of a child view. When a view is resolved we also resolve all its parents and add their columns (see MetadataEndpointImpl.addDerivedColumnsFromAncestors).
I added some more tests for salted tables. I also changed PhoenixDatabaseMetaData.getPrimaryKeys() to load the PTable and return the metadata instead of directly querying the SYSTEM.CATALOG table (which will not work any more since we don't store columns inherited from parents in a view)

@ankitsinghal @ChinmaySKulkarni can you please review?

@twdsilva
Copy link
Contributor Author

ping @ChinmaySKulkarni @karanmehta93 any chance I can get this reviewed soon?

@ChinmaySKulkarni
Copy link
Contributor

@twdsilva Had a quick glance over the PR. I had a few questions. So in the case a view is created on top of a salted base table, we explicitly filter out the salting column (first column) when resolving the columns for a view, right? Also, in case any ancestor of a view drops a column, that will be part of the excluded columns list and we filter out those columns as well, when resolving a view? What happens if a view diverges from the base table, the base table drops a column and then the diverged view re-adds this column?

Overall, can you please add more comments to make it easier to understand the steps taken when handling excluded columns? I will go over the PR in detail and let you know some more feedback. Thanks

@ChinmaySKulkarni
Copy link
Contributor

A few comments. Please also address the test failures inside ViewIT.java. Otherwise, lgtm.

Copy link
Contributor Author

@twdsilva twdsilva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChinmaySKulkarni Thanks for the review, I have updated the PR. I will commit this to 4.x and master branches.

@twdsilva twdsilva force-pushed the PHOENIX-4855 branch 2 times, most recently from b9d5fbe to 7a22b26 Compare October 4, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants