You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@phoenix.apache.org by "Christine Feng (Jira)" <ji...@apache.org> on 2020/03/06 19:40:00 UTC
[jira] [Updated] (PHOENIX-5432) Refactor LiteralExpression to use
the builder pattern
[ 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
NOTE: No backward compatibility testing to be done on master branch, waiting on review
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
> 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
> NOTE: No backward compatibility testing to be done on master branch, waiting on review
>
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)