use Calcites.getColumnTypeForRelDataType for SQL CAST operator conversion#13890
use Calcites.getColumnTypeForRelDataType for SQL CAST operator conversion#13890gianm merged 3 commits intoapache:masterfrom
Conversation
| DruidExpression.ofColumn(ColumnType.LONG, "t"), | ||
| // RexNode type "interval year to month" is not reported as ColumnType.STRING | ||
| DruidExpression.ofLiteral(null, DruidExpression.stringLiteral("P13M")), | ||
| DruidExpression.ofLiteral(ColumnType.LONG, DruidExpression.stringLiteral("P13M")), |
There was a problem hiding this comment.
Long type for string literal? Seems not right.
There was a problem hiding this comment.
hmm, so this was a result of merging the rules from cast into Calcites.getColumnTypeForRelDataType which had interval types as LONG, but agree it does look a bit strange so will fix to report these interval types as strings instead and see what that breaks
There was a problem hiding this comment.
ah, i see why it was treating the cast as LONG typed...
timestampDiff test for example starts spitting out explicit casts which are not necessary so I think i need to add a bit extra logic to cast to let intervals cast to longs without a cast ExpressionVirtualColumn{name='v0', expression='div(CAST(("__time" - 915148800000), 'LONG'),86400000)', outputType=LONG}
There was a problem hiding this comment.
ok, updated to handle intervals as STRINGS so they match the argument literals in plans, but also handle them as longs for casting (and also reduction). I suppose instead I could just drop intervals from being handled by Calcites.getColumnTypeForRelDataType like it was before and just handle them in the cast, but string seems the most correct i think?
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM after the latest changes
…sion (apache#13890) * use Calcites.getColumnTypeForRelDataType for SQL CAST operator conversion * fix comment * intervals are strings but also longs
3 participants
Description
This PR modifies the SQL
CASToperator conversion to useCalcites.getColumnTypeForRelDataTypeto convert calcite types to native Druid types instead of using its own customSqlTypeNametoExprTypemapping to make it more consistent with other SQL to Druid type conversions done for most other operators. This allows it to handle some additional casts which were not previously supported, such as those with ARRAY types. The test added toCalciteMultiValueStringQueryTestwas not able to be planned prior to the changes to theCASToperator since during planning explicit casts would be added which would then fail since the mapping table was missing arrays.Switching
CASTto useCalcites.getColumnTypeForRelDataTypealso required making a small adjustment to this method to be able to handleSqlTypeName.DAY_INTERVAL_TYPESandSqlTypeName.YEAR_INTERVAL_TYPESas a DruidLONGtype. This also allows some additional expressions to be planned that were not previously supported, shown in the changes toExpressionsTest,GreatestExpressionTest, andLeastExpressionTest.This PR has: