PHOENIX-4996: Refactor PTableImpl to use Builder Pattern

[ChinmaySKulkarni]

• Added builder pattern
• Removed unnecessary constructors and makePTable methods

[ChinmaySKulkarni]

@twdsilva @vincentpoon please review.

Just a note: On top of the changes mentioned in my PR, I have simplified the init method. However, I've decided against removing it since there are attributes being set inside it which depend on other attributes already being set, etc. Let me know if you guys have a different opinion or a better way of designing this.

[twdsilva]

@ChinmaySKulkarni looks pretty good. Since we always call the init() method, it should be possible to just call the corresponding setter methods (for tenantId, schemaName etc) and then make init() private and only call it from the build() right? If any of the required fields haven't been set then we could just throw an exception in the build().

[twdsilva]

After talking with Chinmay offline, I don't think its possible to move init() into the build() as its not always being called. +1 assuming the tests pass.

[vincentpoon]

Instead of passing in override props, why not just return a Builder?
e.g. instead of
Map<String, Object> overrideProps = new HashMap<>(); overrideProps.put(PTableImpl.COLUMNS, projectedColumns); PTable t = PTableImpl.makePTableFromExisting(table, overrideProps);

how about something like
PTable t = PTableImpl.builderFromExisting(table).setColumns(projectedColumns).build();

[ChinmaySKulkarni]

@vincentpoon the problem with that approach is that there are certain fields that are set inside init and changing those fields would require us to change other fields, for example, changing the columns would require us to recompute the estimatedSize, pkColumns, etc. so it's not sufficient to return the builder and then just set whatever are the new properties. On top of this, the developer would need to keep track of the fact that init needs to be called again after some fields are changed, which is error-prone. I feel that we should keep this abstracted away. Let me know what you think.

[twdsilva]

It would be cleaner if we don't need the override props map. Currently makePTableFromExisting calls init() and then build() using all the fields from the passed in table except those that are overridden.
Would it work if we return a builder from the passed in PTable call the setter to override whichever properties and always call init() and then build() ?

[ChinmaySKulkarni]

• Removed the override props map
• Now using builderWithColumns which calls builderFromExisting instead
  of the earlier makePTableFromExisting
• Renamed init to initDerivedAttributes
• initDerivedAttributes is called from the build method when columns are
  set in the builder.

Overall, now all places use only setters/builderWithColumns and directly call build without having to explicitly call init or any such equivalent method. Let me know what you guys think @twdsilva @vincentpoon

[twdsilva]

Nice work @ChinmaySKulkarni , LGTM will commit after the precommit build is run.

[vincentpoon]

+1, thanks @ChinmaySKulkarni

[ChinmaySKulkarni]

Thanks @twdsilva and @vincentpoon for the reviews. I've attached the patch for master on the JIRA