Thanks for the follow-up commit, https://git-wip-us.apache.org/repos/asf?p=calcite.git;a=commit;h=37944bb638f2d3c52f884a841c2a27532e892014 <https://git-wip-us.apache.org/repos/asf?p=calcite.git;a=commit;h=37944bb638f2d3c52f884a841c2a27532e892014>.
I saw that you renamed the config parameter NULL_IS_EMPTY to NULL_EQUAL_TO_EMPTY. The problem I had — sorry I didn’t make it clear — was that parameter applies only to the Druid adapter, but people would assume that it applied to the whole of Calcite. (There are databases that treat empty string as null — e.g. Oracle — so people are very likely to assume that we are doing the same.) Could you please rename it to DRUID_NULL_EQUAL_TO_EMPTY or DRUID_NULL_IS_EMPTY? Julian > On Jun 14, 2018, at 7:36 AM, Julian Hyde <[email protected]> wrote: > > 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) { >> >
