-
Notifications
You must be signed in to change notification settings - Fork 63
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
Adds int32 and int64 variants of date_add #1291
Conversation
Conformance comparison report
Number passing in both: 5384 Number failing in both: 434 Number passing in Base (1c3c79a) but now fail: 0 Number failing in Base (1c3c79a) but now pass: 0 |
@@ -590,23 +590,26 @@ public object PartiQLHeader : Header() { | |||
private fun extract(): List<FunctionSignature.Scalar> = emptyList() | |||
|
|||
private fun dateAdd(): List<FunctionSignature.Scalar> { | |||
val intervals = listOf(INT32, INT64, INT) |
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.
val intervals = listOf(INT32, INT64, INT) | |
val intervals = listOf(INT8, INT16, INT32, INT64, INT) |
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.
That was intentional. They're not necessary because the INT8 and INT16 will be implicitly cast to INT32
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.
What if the input is not a literal? Would you still prefer an implicit cast?
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.
DateAdd(Day, a_int16_type, b_timestamp_type)
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.
Yeah that's fine. We mostly need int32 for our target systems
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.
The default was INT which should never be the default. The default integer should always be INT32. Then INT64 is between them, hence why I did those three
* Adds support for EXCLUDE in the SqlDialect * fix typer/transfomrer * Fix Timestamp Type Parsing Issue. (#1284) * fix timestamp type parsing issue * Refactors partiql-tests-runner for multiple engines (#1289) * Adds int32 and int64 variants of date_add (#1291) * Adds support for DISTINCT in the Planner * Fix PlanCompiler output type for DISTINCT + ORDER BY (#1298) * Fixes scan_indexed and adds PIVOT (#1297) * Port filter_distinct fix to partiql-eval internal --------- Co-authored-by: John Ed Quinn <johqunn@amazon.com> Co-authored-by: Yingtao Liu <yliuuu@amazon.com> Co-authored-by: yliuuuu <107505258+yliuuuu@users.noreply.github.com> Co-authored-by: John Ed Quinn <40360967+johnedquinn@users.noreply.github.com> Co-authored-by: R. C. Howell <RCHowell@users.noreply.github.com>
Relevant Issues
N/A
Description
Adds int32 and int64 operators to the date_add function. Before we only had the arbitrary-sized integer which we don't want as default.
Other Information
Updated Unreleased Section in CHANGELOG: [YES/NO]
No
Any backward-incompatible changes? [YES/NO]
No
Any new external dependencies? [YES/NO]
No
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]
Yes
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.