Cast Impl & Char_length & Abs function #1363
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 435 Number passing in legacy engine but fail in eval engine: 2191 Number failing in legacy engine but pass in eval engine: 3 Click here to see
Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 438 Number passing in Base (97f9926) but now fail: 0 Number failing in Base (97f9926) but now pass: 0 Conformance comparison report-Cross Commit-EVAL
Number failing in both: 2626 Number passing in Base (97f9926) but now fail: 0 Number failing in Base (97f9926) but now pass: 1682 |
| override fun toInt8(): Int8Value { | ||
| val byte = this.value?.toByte() ?: return int8Value(null, annotations) | ||
| if (byte.toShort() != this.value) { | ||
| throw DataException("Overflow when casting ${this.value} to INT8") | ||
| } | ||
| return int8Value(byte, annotations) | ||
| } | ||
|
|
||
| override fun toInt16(): Int16Value = this | ||
|
|
||
| override fun toInt32(): Int32Value = int32Value(this.value?.toInt(), annotations) | ||
|
|
||
| override fun toInt64(): Int64Value = int64Value(this.value?.toLong(), annotations) | ||
|
|
||
| override fun toInt(): IntValue = intValue(this.value?.toLong()?.let { BigInteger.valueOf(it) }, annotations) | ||
|
|
||
| override fun toDecimal(): DecimalValue = decimalValue(this.value?.toBigDecimal(), annotations) | ||
|
|
||
| override fun toFloat32(): Float32Value = float32Value(this.value?.toFloat(), annotations) | ||
|
|
||
| override fun toFloat64(): Float64Value = float64Value(this.value?.toDouble(), annotations) |
There was a problem hiding this comment.
Random note, and I don't think we need to change anything here. Just wanted to call out that we are preserving annotations on the cast.
| GeneratedParser.MOD -> exprBinary(Expr.Binary.Op.MODULO, args[0], args[1]) | ||
| else -> visitNonReservedFunctionCall(ctx, args) |
There was a problem hiding this comment.
I'm very curious to explore why we have the reserved function names at all. Other SQL ANTLR grammars simply use a qualified name. I don't think we should change it here, but let's check with John as to why he included those.
There was a problem hiding this comment.
Agree. Maybe we can have an alias parameter in FnSignature or something to resolve the multiple naming issue. (i.e., char_length and character_length)
| @@ -145,13 +147,32 @@ class EvalExecutor( | |||
| private fun infer(env: StructElement): Connector { | |||
| val map = mutableMapOf<String, StaticType>() | |||
| env.fields.forEach { | |||
| map[it.name] = StaticType.ANY | |||
| map[it.name] = inferEnv(it.value) | |||
| val arg = arg.eval(record) | ||
| try { | ||
| return when (arg.type) { |
There was a problem hiding this comment.
This is ok for now, but considering the planner knows the argument input type we can actually select a branch at compile-time rather than runtime.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## partiql-eval #1363 +/- ##
===============================================
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. |
Relevant Issues
Description
Other Information
Updated Unreleased Section in CHANGELOG: [YES/NO]
Any backward-incompatible changes? [YES/NO]
Any new external dependencies? [YES/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.