Optimizes dynamic dispatch & casts. Fixes arithmetic operations, casts, and more. #1533
Conversation
e74ae4a to
935fcfa
Compare
| @@ -226,7 +225,6 @@ internal class MainCommand : Runnable { | |||
| } | |||
| val catalog = MemoryCatalog.builder() | |||
| .name("default") | |||
| .info(InfoSchema.ext()) | |||
There was a problem hiding this comment.
This is just a fix from merging in recent changes from the CLI and SPI changes.
There was a problem hiding this comment.
This is added because there is one conformance test that requires IS TUPLE that passes in the old evaluator. We previously didn't have this function.
There was a problem hiding this comment.
You should change the RexConverter to have IS TUPLE lower to is_struct since these are the same function.
| @NotNull | ||
| static Datum intArbitrary(@NotNull BigInteger value) { | ||
| return new DatumBigInteger(value); | ||
| } |
There was a problem hiding this comment.
This is mixing C-style names with SQL names. I say go for all SQL names for symmetry with PType. So update update int64Value to bigint.
Or visa-versa! No strong preference, just that right now they're mixed
There was a problem hiding this comment.
Yeah, I was tempted to change all of them to SQL as I ran through this PR, though I held back due to the diff. The intention is to use the SQL names as it's the public API. If you don't mind, my follow-up PRs aim to clean up some of our APIs.
partiql-eval/src/main/java/org/partiql/eval/value/DatumCollection.java
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/Compiler.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/CastTable.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/CastTable.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt
Outdated
Show resolved
Hide resolved
partiql-eval/src/main/kotlin/org/partiql/eval/internal/operator/rex/ExprCallDynamic.kt
Show resolved
Hide resolved
| private val paramIndices: IntRange = args.indices | ||
| private val paramTypes: List<List<PType>> = this.candidates.map { candidate -> candidate.fn.signature.parameters.map { it.type } } | ||
| private val paramFamilies: List<List<CoercionFamily>> = this.candidates.map { candidate -> candidate.fn.signature.parameters.map { family(it.type.kind) } } | ||
| private val cachedMatches: MutableMap<List<PType>, Int> = mutableMapOf() |
There was a problem hiding this comment.
Interesting that this is a MutableMap<List<PType>> rather than something like MutableMap<List<PType.Kind>>.
There was a problem hiding this comment.
It has to do with our function resolution. We follow PostgreSQL for exact matches -- which take into account parameters. So, in one dispatch (say argument of type DECIMAL(2,0)) could be matched to one candidate, while another dispatch (say argument DECIMAL(3,0)) could be matched to another candidate.
If we change our modelling of function signatures, then this can change as well.
Fixes arithmetic operations, casts, and more.
935fcfa to
d1a0506
Compare
| @@ -251,7 +251,7 @@ internal class Compiler( | |||
| error("Dynamic call candidate had different name than others; found $fnName but expected $name") | |||
| } | |||
| } | |||
| ExprCallDynamic.Candidate(fn) | |||
| fn | |||
There was a problem hiding this comment.
This is due to making ExprCallDynamic.Candidate private.
Preface
Description
FnResolverto see how we pass along the invocable matches when we have any dynamic argument (so we can see which has the most exact matches at runtime). Also, seeExprCallDynamic-- it is now simplified to run through the sorted candidates. See the KDocs there.ExprCast. It now internally uses aCastTablewhich is a two-dimensional array (indexed by thePType.Kindordinals) to receive a cast function. This speeds up the casts, and it's also much easier to read. It retains the same functionality as before while simplifying it and speeding it up.What's Next?
Fns to useDatuminstead. With this, our performance should be substantially better.Other Information
and Code Style Guidelines? YES
License Information
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.