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, [email protected] 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 <[email protected]> > Authored: Tue Jun 12 10:38:56 2018 +0100 > Committer: Nishant <[email protected]> > 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) { >
