You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by ChinmaySKulkarni <gi...@git.apache.org> on 2018/10/30 20:41:23 UTC
[GitHub] phoenix pull request #401: PHOENIX-4996: Refactor PTableImpl to use Builder ...
GitHub user ChinmaySKulkarni opened a pull request:
https://github.com/apache/phoenix/pull/401
PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
- Added builder pattern
- Removed unnecessary constructors and `makePTable` methods
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/ChinmaySKulkarni/phoenix PHOENIX-4996
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/401.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #401
----
commit e2e58fd5be5a61b53cf4e81b46b89de084e0f23d
Author: Chinmay Kulkarni <ch...@...>
Date: 2018-10-25T00:56:22Z
PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
----
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/401
- 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
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on the issue:
https://github.com/apache/phoenix/pull/401
+1, thanks @ChinmaySKulkarni
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/401
@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().
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/401
Nice work @ChinmaySKulkarni , LGTM will commit after the precommit build is run.
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/401
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() ?
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/401
Thanks @twdsilva and @vincentpoon for the reviews. I've attached the patch for master on the JIRA
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by vincentpoon <gi...@git.apache.org>.
Github user vincentpoon commented on the issue:
https://github.com/apache/phoenix/pull/401
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();`
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/401
I also had to bump up the spark version to 2.3.2 as this version has more sql support, in order to get the tests to pass.
---
[GitHub] phoenix pull request #401: PHOENIX-4996: Refactor PTableImpl to use Builder ...
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni closed the pull request at:
https://github.com/apache/phoenix/pull/401
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by twdsilva <gi...@git.apache.org>.
Github user twdsilva commented on the issue:
https://github.com/apache/phoenix/pull/401
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.
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/401
@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.
---
[GitHub] phoenix issue #401: PHOENIX-4996: Refactor PTableImpl to use Builder Pattern
Posted by ChinmaySKulkarni <gi...@git.apache.org>.
Github user ChinmaySKulkarni commented on the issue:
https://github.com/apache/phoenix/pull/401
@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 user 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 from the user. Let me know what you think.
---