You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2021/12/22 17:11:11 UTC
[GitHub] [druid] abhishekagarwal87 opened a new pull request #12092: Fix SQL queries for inline datasource with null values
abhishekagarwal87 opened a new pull request #12092:
URL: https://github.com/apache/druid/pull/12092
Fixes a bug because of which some SQL queries cannot be parsed using druid convention. Specifically, these queries translate to an inline datasource and have some null values. Calcite internally uses NULL as SQL type for these literals and that is not supported by the druid. In this PR, we are marking such null literals as the STRING column type.
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla edited a comment on pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla edited a comment on pull request #12092:
URL: https://github.com/apache/druid/pull/12092#issuecomment-1004065242
Minor comment, thanks for addressing the doubts.
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla edited a comment on pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla edited a comment on pull request #12092:
URL: https://github.com/apache/druid/pull/12092#issuecomment-1004065242
Just a small doubt. Thanks for addressing the comments.
LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r782784383
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java
##########
@@ -173,6 +173,8 @@ public static ColumnType getValueTypeForRelDataTypeFull(final RelDataType type)
return ColumnType.LONG_ARRAY;
}
return ColumnType.STRING_ARRAY;
+ } else if (sqlTypeName == SqlTypeName.NULL) {
+ return ColumnType.STRING;
Review comment:
Correction - RowSignature allows non-null column type. The exception is thrown during the conversion of signature from calcite type to druid type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777452531
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
It seems I mixed up `literal.getType().getSqlTypeName()` and `literal.getSqlTypeName()` when I originally wanted to ask the question.
In the current modification, instead of returning `null`, should it be `throw new UnsupportedSQLQueryException("%s type is not supported", literal.getType().getSqlTypeName());`, (default behavior) to preserve the original behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on pull request #12092:
URL: https://github.com/apache/druid/pull/12092#issuecomment-1003983260
@LakshSingla thank you for your review. Please let me know if you feel there is a code change required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r782781872
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java
##########
@@ -173,6 +173,8 @@ public static ColumnType getValueTypeForRelDataTypeFull(final RelDataType type)
return ColumnType.LONG_ARRAY;
}
return ColumnType.STRING_ARRAY;
+ } else if (sqlTypeName == SqlTypeName.NULL) {
+ return ColumnType.STRING;
Review comment:
```
org.apache.druid.java.util.common.ISE: Cannot translate sqlTypeName[NULL] to Druid type for field[EXPR$0]
at org.apache.druid.sql.calcite.table.RowSignatures.fromRelDataType(RowSignatures.java:68)
at org.apache.druid.sql.calcite.rule.DruidLogicalValuesRule.onMatch(DruidLogicalValuesRule.java:75)
at org.apache.calcite.plan.volcano.VolcanoRuleCall.onMatch(VolcanoRuleCall.java:208)
```
We want to build the DruidTable object with a valid RowSignature from the inline datasource. RowSignature needs a non-null column type for each column. When there is just row with null value in the inline data, the sql type is null. When we don't have a corresponding druid column type, we throw the above exception.
Instead of introducing a new column type, I have mapped the null type to string type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] clintropolis commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r779405728
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/planner/Calcites.java
##########
@@ -173,6 +173,8 @@ public static ColumnType getValueTypeForRelDataTypeFull(final RelDataType type)
return ColumnType.LONG_ARRAY;
}
return ColumnType.STRING_ARRAY;
+ } else if (sqlTypeName == SqlTypeName.NULL) {
+ return ColumnType.STRING;
Review comment:
why is this needed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on pull request #12092:
URL: https://github.com/apache/druid/pull/12092#issuecomment-1004065242
Minor doubt, thanks for addressing the comments. LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777383490
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
If it was, wouldn't we see an unsupported type exception? do you mean something different
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777467643
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
that's what it was doing earlier which means we can't support null literals in SQL queries.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r775039036
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java
##########
@@ -242,6 +242,7 @@ private static DruidExpression rexCallToDruidExpression(
.lookupOperatorConversion(operator);
if (conversion == null) {
+ plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName());
Review comment:
Since this method is mostly used as a helper/util method, setting the error here might not be desirable in every use case. Would it be better if it is moved to a place like this? [Projections, line 105](https://github.com/apache/druid/blob/476d0bf4be4199e97695bd568d165cda98523d37/sql/src/main/java/org/apache/druid/sql/calcite/rel/Projection.java#L105)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777382324
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/expression/Expressions.java
##########
@@ -242,6 +242,7 @@ private static DruidExpression rexCallToDruidExpression(
.lookupOperatorConversion(operator);
if (conversion == null) {
+ plannerContext.setPlanningError("SQL query requires '%s' operator that is not supported.", operator.getName());
Review comment:
the absence of a corresponding operator seems like a genuine feature gap. Also, I can populate a more precise error here than I can in the uppar part of the call stack. Since a null can be returned for other reasons.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777467780
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
this change fixes that exception.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r775037207
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
Could this have been the case when Boolean is of null type?
Referring to this conversation: https://github.com/apache/druid/pull/11923#discussion_r766485610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777452531
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
It seems I mixed up `literal.getType().getSqlTypeName()` and `literal.getSqlTypeName()` when I originally wanted to ask the question.
In the current modification, instead of returning `null`, should it be `throw new UnsupportedSQLQueryException("%s type is not supported", literal.getType().getSqlTypeName());`, to preserve the original behavior.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] LakshSingla commented on a change in pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
LakshSingla commented on a change in pull request #12092:
URL: https://github.com/apache/druid/pull/12092#discussion_r777480523
##########
File path: sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidLogicalValuesRule.java
##########
@@ -128,6 +128,11 @@ static Object getValueFromLiteral(RexLiteral literal, PlannerContext plannerCont
case TIMESTAMP:
case DATE:
return Calcites.calciteDateTimeLiteralToJoda(literal, plannerContext.getTimeZone()).getMillis();
+ case NULL:
Review comment:
Got it, thanks. I was incorrectly assuming that this was aimed at making the error message more specific to null columns.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] abhishekagarwal87 merged pull request #12092: Fix SQL queries for inline datasource with null values
Posted by GitBox <gi...@apache.org>.
abhishekagarwal87 merged pull request #12092:
URL: https://github.com/apache/druid/pull/12092
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org