opt: corrects function doc for canMaybeConstrainIndex #47232
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @mgartner)
pkg/sql/opt/xform/custom_funcs.go, line 986 at r1 (raw file):
} // canMaybeConstrainIndex can quickly rule out the possibility that the given
[nit] I would keep "performs checks", the new description suggests the opposite semantics
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status:
pkg/sql/opt/xform/custom_funcs.go, line 992 at r1 (raw file):
// // 1. The filter references the first index column. // 2. The constraints are not tight (see Scalar.TightConstraints).
[nit] I'd say props.Scalar.TightConstraints since props is a different package than this one.
mgartner
force-pushed
the
correct-can-maybe-constrain-index-comment
branch
from
5e23169 to
0e822c4
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/custom_funcs.go, line 986 at r1 (raw file):
Previously, RaduBerinde wrote…
[nit] I would keep "performs checks", the new description suggests the opposite semantics
I updated this first sentence. Let me know if this is ok. Happy to adjust until it is clear.
The new description is intentionally the opposite of what it was before, because the function returns true if any of the checks are true. So "ruling out" seemed not accurate. It is ruled out of all of the checks fail.
pkg/sql/opt/xform/custom_funcs.go, line 992 at r1 (raw file):
Previously, rytaft (Rebecca Taft) wrote…
[nit] I'd say
props.Scalar.TightConstraintssincepropsis a different package than this one.
👍
|
❌ The GitHub CI (Cockroach) build has failed on 0e822c44. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan. |
|
pkg/sql/opt/xform/custom_funcs.go, line 986 at r1 (raw file): Previously, mgartner (Marcus Gartner) wrote…
"X can rule out" suggests that X returns true when it rules out something. I'd say "canMaybeCobstrainIndex returns ttrue if we should try to constrain a given index. It can quickly rule out.." Up to you though, I realize I'm bikeshedding here |
This commit corrects the function documentation for canMaybeConstrainIndex. Prior to this change, it did not mention a third check, for tight filter constraints, that is performed to determine the possibility that an index could be constrained by a filer. Also, the documentation now correctly maps to the logic of the function. Previously it falsely claimed that if any of the checks were false then the function would return false. Now it correctly states that if any of the checks are true, then the function returns true. Release note: None
mgartner
force-pushed
the
correct-can-maybe-constrain-index-comment
branch
from
0e822c4 to
c6423e9
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/xform/custom_funcs.go, line 986 at r1 (raw file):
Previously, RaduBerinde wrote…
"X can rule out" suggests that X returns true when it rules out something. I'd say "canMaybeCobstrainIndex returns ttrue if we should try to constrain a given index. It can quickly rule out.." Up to you though, I realize I'm bikeshedding here
I agree with you. I took out the "rule out" language entirely to avoid confusion. Now it more clearly state when it returns true and when it returns false.
There was a problem hiding this comment.
Reviewable status:
|
bors r+ |
Build succeeded |
opt: corrects function doc for canMaybeConstrainIndex
This commit corrects a the function documentation for
canMaybeConstrainIndex.
Prior to this change, it did not mention a third check, for tight filter
constraints, that is performed to determinte the possibility that an
index could be constrained by a filer.
Also, the documentation now correctly maps to the logic of the function.
Previously it falsly claimed that if any of the checks were false then
the function would return false. Now it correctly states that if any of
the checks are true, then the fucntion returns true.
Release note: None