-
Notifications
You must be signed in to change notification settings - Fork 62
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
Additional Functions implementation for Eval Engine #1375
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## partiql-eval #1375 +/- ##
===============================================
Coverage ? 50.32%
Complexity ? 1045
===============================================
Files ? 165
Lines ? 13129
Branches ? 2452
===============================================
Hits ? 6607
Misses ? 5862
Partials ? 660
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Conformance comparison report-Cross Engine
Number failing in both: 264 Number passing in eval engine but fail in legacy engine: 172 Number failing in eval engine but pass in legacy engine: 858 Conformance comparison report-Cross Commit-EVAL
Number failing in both: 1122 Number passing in Base (d749103) but now fail: 0 Number failing in Base (d749103) but now pass: 211 Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 436 Number passing in Base (d749103) but now fail: 0 Number failing in Base (d749103) but now pass: 0 |
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.
Overall looks great. Some minor comments.
partiql-planner/src/main/kotlin/org/partiql/planner/internal/transforms/RexConverter.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/builtins/FnCardinality.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/builtins/FnCardinality.kt
Outdated
Show resolved
Hide resolved
override fun invoke(args: Array<PartiQLValue>): PartiQLValue { | ||
val v = args[0].check<TimeValue>().value!! | ||
return when (val tz = v.timeZone) { | ||
TimeZone.UnknownTimeZone -> int32Value(0) |
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.
QQ: You're the one most familiar with date time semantics, but should this be 0? While it's not a boolean value, in boolean logic, an unknown value gets coerced to a NULL value.
The Ion spec says:
2007-02-23T20:14:33.079Z // The same instant in UTC ("zero" or "zulu")
2007-02-23T20:14:33.079+00:00 // The same instant, with explicit local offset
2007-02-23T20:14:33.079-00:00 // The same instant, with unknown local offset
Which shows that the timestamps themselves are equivalent. But, if you were to want to derive information from your data, I feel as if NULL would be returned upon extracting an unknown offset.
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.
This is a good question.
While I don't know what should the decision be, for the time being I decided to follow the behavior from the existing pipelines, which returns zero (in form of decimal for some reasons)...
EXTRACT(TIMEZONE_HOUR FROM `2024-01-01T23:12:59-00:00`)
|
==='
0.
I think for the time being we can stay with that behavior, but I will add a todo note for this line.
Great call out. Thanks.
partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/exts/FnExists.kt
Outdated
Show resolved
Hide resolved
partiql-spi/src/main/kotlin/org/partiql/spi/connector/sql/exts/FnSize.kt
Outdated
Show resolved
Hide resolved
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.
LGTM!
Relevant Issues
Description
Other Information
Updated Unreleased Section in CHANGELOG: [YES/NO]
Any backward-incompatible changes? [YES/NO]
errors for users that are using our public APIs or the entities that have
public
visibility in our code-base. >Any new external dependencies? [YES/NO]
Do your changes comply with the Contributing Guidelines
and Code Style Guidelines? [YES/NO]
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.