phoenix-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christine Feng (Jira)" <j...@apache.org>
Subject [jira] [Updated] (PHOENIX-5432) Refactor LiteralExpression to use the builder pattern
Date Tue, 14 Apr 2020 02:06:00 GMT

     [ https://issues.apache.org/jira/browse/PHOENIX-5432?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]

Christine Feng updated PHOENIX-5432:
------------------------------------
    Description: 
LiteralExpression is a mess. While it provides newConstant() APIs to build the object, it
also provides two public constructors. There are 10 overloaded newConstant() methods and it
is unclear which API to use in which case.

This should be refactored to use the builder pattern and final member variables. Ideally,
getters such as getMaxLength() should be simple member variable accessors and other ad-hoc
logic surrounding those variables should be handled correctly when setting their respective
values.

 

Two solutions:
 # -Consolidate the LiteralExpression newConstant() methods down into a single build() method-
 ** -Pros: easy to use since one build method can create all LiteralExpression objects-
 ** -Cons: requires adding 'throws SQLException' to a lot of method signatures where it wasn't
necessary before, which can be confusing for future developers and potentially cause problems
if a SQLException is actually thrown in some of these cases-
 # -Create two build() methods - one for LiteralExpressions that necessitate deriving value
(which could throw SQLExceptions) and ones that don't-
 ** -Pros: don't need to change any existing method signatures-
 ** -Cons: requires future developers to know which of the two build methods to use-
 # Create two Builders for two separate use cases, each with its own build method.
 ## Pros: less possibility for confusion
 ## Cons: needs very explicit documentation on which Builder to use in which case

NOTE (13 April 2020): Putting this JIRA on hold for a few weeks until I get the bandwidth
to come back to it; for reference, was in the process of switching over from my current Builder
with two build methods to two distinct Builders, with one build method each, for distinct
and ideally well-documented use cases.

GOAL: The goal for this Jira is to simplify the LiteralExpression class and make it easier
for future contributors to understand and use LiteralExpression objects.


ORIGINAL CODE BEHAVIOR: The original LiteralExpression class had a lot of LiteralExpression
constructors, as well as newConstant helper methods that modified some set of LiteralExpression
attributes before passing the values onto a constructor. All in all, the various constructors
and helper methods boiled down to two types of LiteralExpression creation cases: those that
necessitate throwing a SQLException (Case A), and those that do not (Case B).

PROPOSED REDESIGN: By creating one LiteralExpressionBuilder that handles Case A, and another
that handles Case B, and with sufficient documentation, we avoid possible confusion and resultant
creation of LiteralExpression objects with incorrect or unintended attributes. 

  was:
LiteralExpression is a mess. While it provides newConstant() APIs to build the object, it
also provides two public constructors. There are 10 overloaded newConstant() methods and it
is unclear which API to use in which case.

This should be refactored to use the builder pattern and final member variables. Ideally,
getters such as getMaxLength() should be simple member variable accessors and other ad-hoc
logic surrounding those variables should be handled correctly when setting their respective
values.

 

Two solutions:
 # -Consolidate the LiteralExpression newConstant() methods down into a single build() method-
 ** -Pros: easy to use since one build method can create all LiteralExpression objects-
 ** -Cons: requires adding 'throws SQLException' to a lot of method signatures where it wasn't
necessary before, which can be confusing for future developers and potentially cause problems
if a SQLException is actually thrown in some of these cases-
 # -Create two build() methods - one for LiteralExpressions that necessitate deriving value
(which could throw SQLExceptions) and ones that don't-
 ** -Pros: don't need to change any existing method signatures-
 ** -Cons: requires future developers to know which of the two build methods to use-
 # Create two Builders for two separate use cases, each with its own build method.
 ## Pros: less possibility for confusion
 ## Cons: needs very explicit documentation on which Builder to use in which case




NOTE (13 April 2020): Putting this JIRA on hold for a few weeks until I get the bandwidth
to come back to it; for reference, was in the process of switching over from my current Builder
with two build methods to two distinct Builders, with one build method each, for distinct
and ideally well-documented use cases.

 

 


> Refactor LiteralExpression to use the builder pattern
> -----------------------------------------------------
>
>                 Key: PHOENIX-5432
>                 URL: https://issues.apache.org/jira/browse/PHOENIX-5432
>             Project: Phoenix
>          Issue Type: Improvement
>    Affects Versions: 4.15.0, 5.1.0
>            Reporter: Chinmay Kulkarni
>            Assignee: Christine Feng
>            Priority: Major
>         Attachments: PHOENIX-5432-master-v1.patch, PHOENIX-5432.master.v10.patch, PHOENIX-5432.master.v11.patch,
PHOENIX-5432.master.v12.patch, PHOENIX-5432.master.v13.patch, PHOENIX-5432.master.v14.patch,
PHOENIX-5432.master.v2.patch, PHOENIX-5432.master.v3.patch, PHOENIX-5432.master.v4.patch,
PHOENIX-5432.master.v5.patch, PHOENIX-5432.master.v6.patch, PHOENIX-5432.master.v7.patch,
PHOENIX-5432.master.v8.patch, PHOENIX-5432.master.v9.patch, PHOENIX-5432.patch
>
>          Time Spent: 6h 20m
>  Remaining Estimate: 0h
>
> LiteralExpression is a mess. While it provides newConstant() APIs to build the object,
it also provides two public constructors. There are 10 overloaded newConstant() methods and
it is unclear which API to use in which case.
> This should be refactored to use the builder pattern and final member variables. Ideally,
getters such as getMaxLength() should be simple member variable accessors and other ad-hoc
logic surrounding those variables should be handled correctly when setting their respective
values.
>  
> Two solutions:
>  # -Consolidate the LiteralExpression newConstant() methods down into a single build()
method-
>  ** -Pros: easy to use since one build method can create all LiteralExpression objects-
>  ** -Cons: requires adding 'throws SQLException' to a lot of method signatures where
it wasn't necessary before, which can be confusing for future developers and potentially cause
problems if a SQLException is actually thrown in some of these cases-
>  # -Create two build() methods - one for LiteralExpressions that necessitate deriving
value (which could throw SQLExceptions) and ones that don't-
>  ** -Pros: don't need to change any existing method signatures-
>  ** -Cons: requires future developers to know which of the two build methods to use-
>  # Create two Builders for two separate use cases, each with its own build method.
>  ## Pros: less possibility for confusion
>  ## Cons: needs very explicit documentation on which Builder to use in which case
> NOTE (13 April 2020): Putting this JIRA on hold for a few weeks until I get the bandwidth
to come back to it; for reference, was in the process of switching over from my current Builder
with two build methods to two distinct Builders, with one build method each, for distinct
and ideally well-documented use cases.
> GOAL: The goal for this Jira is to simplify the LiteralExpression class and make it
easier for future contributors to understand and use LiteralExpression objects.
> ORIGINAL CODE BEHAVIOR: The original LiteralExpression class had a lot of LiteralExpression
constructors, as well as newConstant helper methods that modified some set of LiteralExpression
attributes before passing the values onto a constructor. All in all, the various constructors
and helper methods boiled down to two types of LiteralExpression creation cases: those that
necessitate throwing a SQLException (Case A), and those that do not (Case B).
> PROPOSED REDESIGN: By creating one LiteralExpressionBuilder that handles Case A, and
another that handles Case B, and with sufficient documentation, we avoid possible confusion
and resultant creation of LiteralExpression objects with incorrect or unintended attributes. 



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

Mime
View raw message