-
Notifications
You must be signed in to change notification settings - Fork 980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DRILL-3610: Add TIMESTAMPADD and TIMESTAMPDIFF functions #1528
Conversation
0101a67
to
d798096
Compare
} | ||
} | ||
} | ||
</#list> | ||
</#list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@SuppressWarnings("unused") | ||
@FunctionTemplate(names = {"divide", "div"<#if numerictype == "Int">, "/int"</#if>}, | ||
scope = FunctionTemplate.FunctionScope.SIMPLE, | ||
nulls = NullHandling.NULL_IF_NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to specify somewhere that /int
function (unlike divide
and div
) is for internal use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionTemplate
has isInternal
field for these purposes, but since /int
is used with other non-internal function names, we cant mark it only as internal without splitting it to the separate UDF.
@@ -30,6 +35,29 @@ | |||
@Category({UnlikelyTest.class, SqlFunctionTest.class}) | |||
public class TestDateAddFunctions extends BaseTestQuery { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like TIMESTAMPDIFF
and TIMESTAMPADD
differ from DATE_ADD
, therefore please create a separate class for the new unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, moved them to the separate class.
@@ -61,13 +69,36 @@ | |||
} | |||
}; | |||
|
|||
// Custom convertlet to avoid rewriting TIMESTAMP_DIFF by Calcite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add the small note about the reason why Calcite's TimestampDiffConvertlet
is not suitable for Drill (similar to desc in PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added.
break; | ||
case MICROSECOND: | ||
case MILLISECOND: | ||
// for MICROSECOND and MILLISECOND should be specified precision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace with
precision should be specified for MICROSECOND and MILLISECOND
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, replaced.
@@ -67,6 +67,19 @@ | |||
{input1: "UInt4", input2: "UInt4", outputType: "UInt4", castType: "int"}, | |||
{input1: "UInt8", input2: "UInt8", outputType: "UInt8", castType: "long"} | |||
] | |||
} | |||
}, | |||
{className: "DivideInt", funcName: "/int", op: "/", types: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these functions are nowhere used and can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these UDFs were used in one of the previous version of the fix, so now they can be removed. Thanks for pointing this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vdiravka, I have addressed CR comments, could you please take a look again?
@@ -67,6 +67,19 @@ | |||
{input1: "UInt4", input2: "UInt4", outputType: "UInt4", castType: "int"}, | |||
{input1: "UInt8", input2: "UInt8", outputType: "UInt8", castType: "long"} | |||
] | |||
} | |||
}, | |||
{className: "DivideInt", funcName: "/int", op: "/", types: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, these UDFs were used in one of the previous version of the fix, so now they can be removed. Thanks for pointing this.
@SuppressWarnings("unused") | ||
@FunctionTemplate(names = {"divide", "div"<#if numerictype == "Int">, "/int"</#if>}, | ||
scope = FunctionTemplate.FunctionScope.SIMPLE, | ||
nulls = NullHandling.NULL_IF_NULL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FunctionTemplate
has isInternal
field for these purposes, but since /int
is used with other non-internal function names, we cant mark it only as internal without splitting it to the separate UDF.
} | ||
} | ||
} | ||
</#list> | ||
</#list> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -61,13 +69,36 @@ | |||
} | |||
}; | |||
|
|||
// Custom convertlet to avoid rewriting TIMESTAMP_DIFF by Calcite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, added.
break; | ||
case MICROSECOND: | ||
case MILLISECOND: | ||
// for MICROSECOND and MILLISECOND should be specified precision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, replaced.
@@ -30,6 +35,29 @@ | |||
@Category({UnlikelyTest.class, SqlFunctionTest.class}) | |||
public class TestDateAddFunctions extends BaseTestQuery { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, moved them to the separate class.
7487deb
to
f5e2533
Compare
f5e2533
to
ea96da3
Compare
+1 LGTM |
Added type inference for TIMESTAMPADD function to avoid failures for the cases when function types are checked for expressions before and after rewriting.
Added custom convertlet for TIMESTAMPDIFF function to avoid rewriting it by Calcite, since Drill does not support Reinterpret function and does not handle all Calcite interval representations correctly.
Added UDFs for TIMESTAMPDIFF for all supported time units. The mechanism is similar to the existing EXTRACT function: when Calcite RexCall is converted to the Drill LogicalExpression, a function name is changed to timestampdiffSecond or similar one depending on the specified time unit.
For problem description please see DRILL-3610.