[
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)
