You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by GitBox <gi...@apache.org> on 2021/06/15 22:00:31 UTC
[GitHub] [datasketches-postgresql] AlexanderSaydakov opened a new pull request #41: fixed treatment of default parameters
AlexanderSaydakov opened a new pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] mikeizbicki commented on pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
mikeizbicki commented on pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#issuecomment-863569215
Thanks. I'll defer to your implementation and close the previous PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] jmalkin commented on a change in pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#discussion_r655572534
##########
File path: src/aod_sketch_pg_functions.c
##########
@@ -173,8 +173,7 @@ Datum pg_aod_sketch_get_estimate_and_bounds(PG_FUNCTION_ARGS) {
bytes_in = PG_GETARG_BYTEA_P(0);
sketchptr = aod_sketch_deserialize(VARDATA(bytes_in), VARSIZE(bytes_in) - VARHDRSZ);
- num_std_devs = PG_GETARG_INT32(1);
- if (num_std_devs == 0) num_std_devs = 1; // default
+ num_std_devs = PG_NARGS() > 1 ? PG_GETARG_INT32(1) : 1;
Review comment:
what exactly does `PG_NARGS()` return? This checks if the value is strictly greater than 1, but asks for value 1.
I'm blocked on reviewing this without knowing if that's reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] AlexanderSaydakov commented on a change in pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#discussion_r655621670
##########
File path: src/aod_sketch_pg_functions.c
##########
@@ -173,8 +173,7 @@ Datum pg_aod_sketch_get_estimate_and_bounds(PG_FUNCTION_ARGS) {
bytes_in = PG_GETARG_BYTEA_P(0);
sketchptr = aod_sketch_deserialize(VARDATA(bytes_in), VARSIZE(bytes_in) - VARHDRSZ);
- num_std_devs = PG_GETARG_INT32(1);
- if (num_std_devs == 0) num_std_devs = 1; // default
+ num_std_devs = PG_NARGS() > 1 ? PG_GETARG_INT32(1) : 1;
Review comment:
It returns the number of arguments. So in this case if PG_NARGS() > 1 then there are at least two arguments. Therefore PG_GETARG_XXX(1) should be valid (zero-based index here).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] AlexanderSaydakov commented on pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#issuecomment-866208141
Not sure what safety concern you are talking about. Maybe you are right in a general case, but this seems to work fine.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] AlexanderSaydakov commented on a change in pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov commented on a change in pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#discussion_r655621670
##########
File path: src/aod_sketch_pg_functions.c
##########
@@ -173,8 +173,7 @@ Datum pg_aod_sketch_get_estimate_and_bounds(PG_FUNCTION_ARGS) {
bytes_in = PG_GETARG_BYTEA_P(0);
sketchptr = aod_sketch_deserialize(VARDATA(bytes_in), VARSIZE(bytes_in) - VARHDRSZ);
- num_std_devs = PG_GETARG_INT32(1);
- if (num_std_devs == 0) num_std_devs = 1; // default
+ num_std_devs = PG_NARGS() > 1 ? PG_GETARG_INT32(1) : 1;
Review comment:
It returns the number of arguments. So in this case if PG_NARGS() > 1 then there are at least two arguments. Therefore PG_GETARG_XXX(1) should be valid (zero-based index here).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] AlexanderSaydakov merged pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
AlexanderSaydakov merged pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org
[GitHub] [datasketches-postgresql] jmalkin commented on a change in pull request #41: fixed treatment of default parameters
Posted by GitBox <gi...@apache.org>.
jmalkin commented on a change in pull request #41:
URL: https://github.com/apache/datasketches-postgresql/pull/41#discussion_r655572534
##########
File path: src/aod_sketch_pg_functions.c
##########
@@ -173,8 +173,7 @@ Datum pg_aod_sketch_get_estimate_and_bounds(PG_FUNCTION_ARGS) {
bytes_in = PG_GETARG_BYTEA_P(0);
sketchptr = aod_sketch_deserialize(VARDATA(bytes_in), VARSIZE(bytes_in) - VARHDRSZ);
- num_std_devs = PG_GETARG_INT32(1);
- if (num_std_devs == 0) num_std_devs = 1; // default
+ num_std_devs = PG_NARGS() > 1 ? PG_GETARG_INT32(1) : 1;
Review comment:
what exactly does `PG_NARGS()` return? This checks if the value is strictly greater than 1, but asks for value 1.
I'm blocked on reviewing this without knowing if that's reasonable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
For additional commands, e-mail: commits-help@datasketches.apache.org