You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by ni...@apache.org on 2018/06/14 10:35:47 UTC
calcite git commit: [CALCITE-2358] use null literal instead of empty
string. (b-slim)
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 <sl...@gmail.com>
Authored: Tue Jun 12 10:38:56 2018 +0100
Committer: Nishant <ni...@gmail.com>
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) {
Re: calcite git commit: [CALCITE-2358] use null literal instead of
empty string. (b-slim)
Posted by Julian Hyde <jh...@apache.org>.
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 <jh...@apache.org> 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, nishant@apache.org 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 <sl...@gmail.com>
>> Authored: Tue Jun 12 10:38:56 2018 +0100
>> Committer: Nishant <ni...@gmail.com>
>> 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) {
>>
>
Re: calcite git commit: [CALCITE-2358] use null literal instead of
empty string. (b-slim)
Posted by Julian Hyde <jh...@apache.org>.
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@apache.org 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 <sl...@gmail.com>
> Authored: Tue Jun 12 10:38:56 2018 +0100
> Committer: Nishant <ni...@gmail.com>
> 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) {
>