git.net

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: calcite git commit: [CALCITE-2358] use null literal instead of empty string. (b-slim)


In commit comment: please start with capital letter, no closing period, and don’t include contributor name if the contributor is a committer.

Sorry to nit-pick. I sent out guidelines about this a week or so ago.

And most importantly, make it clear which sub-system this applies to. This change only applies to the Druid adapter, but that is not at all clear from the commit comment. Committers have a duty to make the commit log extremely clear for people reading the release notes and for future maintainers, and this commit falls short.

The name of the config parameter, NULL_IS_EMPTY, is extremely misleading. Please change it.

Lastly, there are no tests for this change. Could there be? 

Julian



> On Jun 14, 2018, at 5:35 AM, nishant@xxxxxxxxxx wrote:
> 
> Repository: calcite
> Updated Branches:
>  refs/heads/master dcf396a5c -> 4536000f2
> 
> 
> [CALCITE-2358] use null literal instead of empty string. (b-slim)
> 
> Close apache/calcite#729
> 
> 
> Project: http://git-wip-us.apache.org/repos/asf/calcite/repo
> Commit: http://git-wip-us.apache.org/repos/asf/calcite/commit/4536000f
> Tree: http://git-wip-us.apache.org/repos/asf/calcite/tree/4536000f
> Diff: http://git-wip-us.apache.org/repos/asf/calcite/diff/4536000f
> 
> Branch: refs/heads/master
> Commit: 4536000f2c4868b75f1000d61a8ac9ed0141fcfa
> Parents: dcf396a
> Author: Slim <slim.bouguerra@xxxxxxxxx>
> Authored: Tue Jun 12 10:38:56 2018 +0100
> Committer: Nishant <nishant.monu51@xxxxxxxxx>
> Committed: Thu Jun 14 16:04:57 2018 +0530
> 
> ----------------------------------------------------------------------
> .../org/apache/calcite/config/CalciteConnectionConfig.java   | 2 ++
> .../apache/calcite/config/CalciteConnectionConfigImpl.java   | 4 ++++
> .../org/apache/calcite/config/CalciteConnectionProperty.java | 5 +++++
> .../apache/calcite/adapter/druid/DruidSqlCastConverter.java  | 8 ++++----
> 4 files changed, 15 insertions(+), 4 deletions(-)
> ----------------------------------------------------------------------
> 
> 
> http://git-wip-us.apache.org/repos/asf/calcite/blob/4536000f/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
> ----------------------------------------------------------------------
> diff --git a/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java b/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
> index 5dd6a1e..ca5fdf1 100644
> --- a/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
> +++ b/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfig.java
> @@ -32,6 +32,8 @@ public interface CalciteConnectionConfig extends ConnectionConfig {
>   boolean approximateTopN();
>   /** @see CalciteConnectionProperty#APPROXIMATE_DECIMAL */
>   boolean approximateDecimal();
> +  /** @see CalciteConnectionProperty#NULL_IS_EMPTY */
> +  boolean nullIsEmpty();
>   /** @see CalciteConnectionProperty#AUTO_TEMP */
>   boolean autoTemp();
>   /** @see CalciteConnectionProperty#MATERIALIZATIONS_ENABLED */
> 
> http://git-wip-us.apache.org/repos/asf/calcite/blob/4536000f/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
> ----------------------------------------------------------------------
> diff --git a/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java b/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
> index 794d1c8..575d7b1 100644
> --- a/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
> +++ b/core/src/main/java/org/apache/calcite/config/CalciteConnectionConfigImpl.java
> @@ -63,6 +63,10 @@ public class CalciteConnectionConfigImpl extends ConnectionConfigImpl
>         .getBoolean();
>   }
> 
> +  @Override public boolean nullIsEmpty() {
> +    return CalciteConnectionProperty.NULL_IS_EMPTY.wrap(properties).getBoolean();
> +  }
> +
>   public boolean autoTemp() {
>     return CalciteConnectionProperty.AUTO_TEMP.wrap(properties).getBoolean();
>   }
> 
> http://git-wip-us.apache.org/repos/asf/calcite/blob/4536000f/core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
> ----------------------------------------------------------------------
> diff --git a/core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java b/core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
> index 67909da..3d765be 100644
> --- a/core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
> +++ b/core/src/main/java/org/apache/calcite/config/CalciteConnectionProperty.java
> @@ -48,6 +48,11 @@ public enum CalciteConnectionProperty implements ConnectionProperty {
>    * DECIMAL types are acceptable. */
>   APPROXIMATE_DECIMAL("approximateDecimal", Type.BOOLEAN, false, false),
> 
> +  /**
> +   * Whether to treat empty strings as null for Druid Adapter.
> +   */
> +  NULL_IS_EMPTY("nullIsEmpty", Type.BOOLEAN, true, false),
> +
>   /** Whether to store query results in temporary tables. */
>   AUTO_TEMP("autoTemp", Type.BOOLEAN, false, false),
> 
> 
> http://git-wip-us.apache.org/repos/asf/calcite/blob/4536000f/druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java
> ----------------------------------------------------------------------
> diff --git a/druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java b/druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java
> index 0731a6f..278bd24 100644
> --- a/druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java
> +++ b/druid/src/main/java/org/apache/calcite/adapter/druid/DruidSqlCastConverter.java
> @@ -56,8 +56,8 @@ public class DruidSqlCastConverter implements DruidSqlOperatorConverter {
> 
>     if (SqlTypeName.CHAR_TYPES.contains(fromType) && SqlTypeName.DATETIME_TYPES.contains(toType)) {
>       //case chars to dates
> -      return castCharToDateTime(timeZone, operandExpression,
> -          toType);
> +      return castCharToDateTime(timeZone, operandExpression, toType,
> +          druidQuery.getConnectionConfig().nullIsEmpty() ? "" : null);
>     } else if (SqlTypeName.DATETIME_TYPES.contains(fromType) && SqlTypeName.CHAR_TYPES.contains
>         (toType)) {
>       //case dates to chars
> @@ -99,13 +99,13 @@ public class DruidSqlCastConverter implements DruidSqlOperatorConverter {
>   private static String castCharToDateTime(
>       TimeZone timeZone,
>       String operand,
> -      final SqlTypeName toType) {
> +      final SqlTypeName toType, String format) {
>     // Cast strings to date times by parsing them from SQL format.
>     final String timestampExpression = DruidExpressions.functionCall(
>         "timestamp_parse",
>         ImmutableList.of(
>             operand,
> -            DruidExpressions.stringLiteral(""),
> +            DruidExpressions.stringLiteral(format),
>             DruidExpressions.stringLiteral(timeZone.getID())));
> 
>     if (toType == SqlTypeName.DATE) {
>