-
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
Handles null calls in dynamic dispatch #1436
Conversation
Conformance comparison report-Cross Engine
Number failing in both: 238 Number passing in legacy engine but fail in eval engine: 341 Number failing in legacy engine but pass in eval engine: 197 Conformance comparison report-Cross Commit-LEGACY
Number failing in both: 435 Number passing in Base (cc05d7f) but now fail: 0 Number failing in Base (cc05d7f) but now pass: 1 Click here to see
Conformance comparison report-Cross Commit-EVAL
Number failing in both: 579 Number passing in Base (cc05d7f) but now fail: 0 Number failing in Base (cc05d7f) but now pass: 8 Click here to see
|
// Check this candidate | ||
val fnArity = fn.signature.parameters.size | ||
val fnName = fn.signature.name | ||
if (arity == -1) { | ||
arity = fnArity | ||
name = fnName | ||
} else { | ||
if (fnArity != arity) { | ||
error("Dynamic call candidate had different arity than others; found $fnArity but expected $arity") | ||
} | ||
if (fnName != name) { | ||
error("Dynamic call candidate had different name than others; found $fnName but expected $name") | ||
} | ||
} |
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 necessary to perform these checks?
Also, we shouldn't be checking the name this way due to case-insensitive lookup. If we need these checks, then I'd consider making var name
maintain a normalized function name.
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, as it's basic one-time input validation. Not strictly necessary if plans come from the internal partiql-planner — but that might not always be where we receive plans from.
Ack on the normalized names.
Description
Handles null calls in dynamic dispatch
Tests Addressed
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.