-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
builtin: add cardinality builtin function #68263
builtin: add cardinality builtin function #68263
Conversation
Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. I have added a few people who may be able to assist in reviewing: 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
thanks for your contribution! it's not exactly equivalent to array_length, just similar. the important difference is the way it handles empty arrays -- if you are able to try it out on postgres you'll see that cardinality returns 0 while array_length returns null. please address that and add a test case |
da56e00
to
7cacbd9
Compare
Thank you for updating your pull request. My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
3eb093a
to
a9fae64
Compare
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 getting close!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jlevesy)
pkg/sql/logictest/testdata/logic_test/builtin_function, line 1787 at r1 (raw file):
SELECT cardinality(ARRAY[]:::int[]) ---- 0
can you add one more test case for SELECT cardinality(NULL::int[])
-- it should return NULL
pkg/sql/sem/builtins/builtins.go, line 3134 at r1 (raw file):
Fn: func(_ *tree.EvalContext, args tree.Datums) (tree.Datum, error) { arr := tree.MustBeDArray(args[0]) return tree.NewDInt(tree.DInt(arr.Len())), nil
sorry to be nitpicky, but i'm worried that if we ever do support multi-dimensional arrays, this function will not work but we won't realize. can you add a recursive implementation? (here's what i'm thinking, borrowing from the arrayLength code)
func cardinality(arr *tree.DArray) tree.Datum {
if arr.ParamTyp.Family() != types.ArrayFamily {
return tree.NewDInt(tree.DInt(arr.Len()))
}
card := 0
for _, a := range arr.Array {
card += int(tree.MustBeDInt(cardinality(tree.MustBeDArray(a))))
}
return tree.NewDInt(tree.DInt(card))
}
Will do! Thanks for your reviews anyway 😄 |
To ensure PostgreSQL compatibility, this commit adds a new builtin function called `cardinality` which returns the total number of elements present in a given array. Fixes: cockroachdb#67996 Release note (sql change): Added cardinality builtin function that returns the total number of elements in a given array.
a9fae64
to
0b53fa8
Compare
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.
thanks for your contribution!
bors r+
Build succeeded: |
To ensure PostgreSQL compatibility, this commit adds a new builtin
function called
cardinality
which returns the total number of elementspresent in a given array.
Fixes: #67996
Release note (sql change): Added cardinality builtin function that
returns the total number of elements in a given array.