[jira] [Comment Edited] (CALCITE-2670) Combine similar JSON aggregate functions in operator table

["Hongze Zhang (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-2670?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16688817#comment-16688817 ] 

Hongze Zhang edited comment on CALCITE-2670 at 11/16/18 12:06 AM:
------------------------------------------------------------------

bq. Why did you convert some parameters from particular types (e.g. SqlJsonQueryEmptyOrErrorBehavior) to untyped Enum?

Thanks [~julianhyde] and I missed explaining this point.

In the patch, I have added SYMBOL-to-Enum type transform to JavaTypeFactoryImpl#getJavaClass. That is, the built-in rel implementer does not know the original type of a SYMBOL.
And rel implementer applies explicitly type casting when translating SYMBOL parameter that is from an aggregate call, so if we transform SYMBOL to Enum in JavaTypeFactoryImpl#getJavaClass, we can only use Enum for the related methods in SqlFunctions.java.

It works before because there were methods like SqlFunctions#jsonObjectAggAddNullOnNull that is for the specific operator like JSON_OBJECTAGG_NULL_ON_NULL, I have deleted these methods in the patch.

And SYMBOL-to-Enum type casting is a trade-off - If we use a JavaRecordType to carry a particular enum type instead of using BasicSqlType, we could use particular types in SqlFunctions.java. But this way needs more code changes, and JavaRecordType is still experimental and not recommended for use.



was (Author: zhztheplayer):
bq. Why did you convert some parameters from particular types (e.g. SqlJsonQueryEmptyOrErrorBehavior) to untyped Enum?

Thanks [~julianhyde] and I missed explaining this point.

In the patch, I have added SYMBOL-to-Enum type transform to JavaTypeFactoryImpl#getJavaClass. That is, the built-in rel implementer does not know the original type of a SYMBOL.
And rel implementer applies explicitly type casting when translating SYMBOL parameter that is from an aggregate call, so if we transform SYMBOL to Enum in JavaTypeFactoryImpl#getJavaClass, we can only use Enum for the related methods in SqlFunctions.java.

It works before because there were methods like SqlFunctions#jsonObjectAggAddNullOnNull that is for the specific operator like JSON_OBJECTAGG_NULL_ON_NULL, I have deleted these methods in the patch.

And SYMBOL-to-Enum type casting is a trade-off - If we use a JavaRecordType to carry a particular enum type instead of using RelDataType, we could use particular types in SqlFunctions.java. But this way needs more code changes, and JavaRecordType is still experimental and not recommended for use.


> Combine similar JSON aggregate functions in operator table
> ----------------------------------------------------------
>
>                 Key: CALCITE-2670
>                 URL: https://issues.apache.org/jira/browse/CALCITE-2670
>             Project: Calcite
>          Issue Type: Improvement
>          Components: core
>            Reporter: Hongze Zhang
>            Assignee: Julian Hyde
>            Priority: Major
>
> In current code master of Calcite, operator *JSON_ARRAYAGG* and *JSON_OBJECTAGG* are separated to *JSON_ARRAYAGG_NULL_ON_NULL*, *JSON_ARRAYAGG_ABSENT_ON_NULL*, *JSON_OBJECTAGG_NULL_ON_NULL* and *JSON_OBJECTAGG_ABSENT_ON_NULL* in SqlStdOperatorTable.java. This is OK for now, but may deliver more difficulty on extending syntax of *JSON_ARRAYAGG* and *JSON_OBJECTAGG* and on implementing logical plan by adapters.
> This is basically to combine the 4 operators to 2, this is a improvement list:
>  # Combine *JSON_ARRAYAGG_NULL_ON_NULL* and *JSON_ARRAYAGG_ABSENT_ON_NULL;*
>  # Combine *JSON_OBJECTAGG_NULL_ON_NULL* and *JSON_OBJECTAGG_ABSENT_ON_NULL;*
>  # *SYMBOL* type Support for *JavaTypeFactoryImpl#getJavaClass*
>  This is to generate *Enum* java type for *SYMBOL*. Current version of Calcite generates *Object[]*, which delivers type casting error, or some method compatible problems {color:#333333}when Calcite tries to pass enum parameters to an aggregate function;{color}
>  # Add SQL-to-Rel test cases for JSON functions.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)