You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datasketches.apache.org by "AlexanderSaydakov (via GitHub)" <gi...@apache.org> on 2023/04/21 21:25:15 UTC
[GitHub] [datasketches-postgresql] AlexanderSaydakov opened a new pull request, #63: Partial aggregation support
AlexanderSaydakov opened a new pull request, #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63
No upgrade scripts yet. I will submit that as a separate pull request.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1177464929
##########
src/hll_sketch_pg_functions.c:
##########
@@ -242,43 +209,105 @@ Datum pg_hll_sketch_get_estimate_from_internal(PG_FUNCTION_ARGS) {
if (PG_ARGISNULL(0)) PG_RETURN_NULL();
if (!AggCheckCallContext(fcinfo, &aggcontext)) {
- elog(ERROR, "hll_sketch_from_internal called in non-aggregate context");
+ elog(ERROR, "hll_sketch_get_estimate_from_internal called in non-aggregate context");
}
oldcontext = MemoryContextSwitchTo(aggcontext);
- sketchptr = PG_GETARG_POINTER(0);
- estimate = hll_sketch_get_estimate(sketchptr);
- hll_sketch_delete(sketchptr);
+ stateptr = (struct hll_agg_state*) PG_GETARG_POINTER(0);
+ estimate = hll_sketch_get_estimate(stateptr->ptr);
+ hll_sketch_delete(stateptr->ptr);
+ pfree(stateptr);
MemoryContextSwitchTo(oldcontext);
PG_RETURN_FLOAT8(estimate);
}
-Datum pg_hll_union_get_result(PG_FUNCTION_ARGS) {
- struct hll_union_state* stateptr;
- void* sketchptr;
- struct ptr_with_size bytes_out;
+Datum pg_hll_sketch_combine(PG_FUNCTION_ARGS) {
+ struct hll_agg_state* stateptr1;
+ struct hll_agg_state* stateptr2;
+ struct hll_agg_state* stateptr;
MemoryContext oldcontext;
MemoryContext aggcontext;
- if (PG_ARGISNULL(0)) PG_RETURN_NULL();
+ if (PG_ARGISNULL(0) && PG_ARGISNULL(1)) PG_RETURN_NULL();
if (!AggCheckCallContext(fcinfo, &aggcontext)) {
- elog(ERROR, "hll_union_get_result called in non-aggregate context");
+ elog(ERROR, "hll_sketch_combine called in non-aggregate context");
}
oldcontext = MemoryContextSwitchTo(aggcontext);
- stateptr = (struct hll_union_state*) PG_GETARG_POINTER(0);
+ stateptr1 = (struct hll_agg_state*) PG_GETARG_POINTER(0);
+ stateptr2 = (struct hll_agg_state*) PG_GETARG_POINTER(1);
+
+ stateptr = palloc(sizeof(struct hll_agg_state));
+ stateptr->type = SKETCH;
+ stateptr->lg_k = stateptr1 ? stateptr1->lg_k : stateptr2->lg_k;
+ stateptr->tgt_type = stateptr1 ? stateptr1->tgt_type : stateptr2->tgt_type;
+ stateptr->ptr = hll_union_new(stateptr->lg_k);
Review Comment:
A comment on why we're creating a union after setting `stateptr->type = SKETCH;` might be useful, even though reading on shows that we overwrite it with the union result.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1176887684
##########
src/aod_sketch_pg_functions.c:
##########
@@ -118,89 +125,43 @@ Datum pg_aod_sketch_add_item(PG_FUNCTION_ARGS) {
}
if (PG_ARGISNULL(0)) {
- lg_k = PG_NARGS() > 3 ? PG_GETARG_INT32(3) : 0;
+ stateptr = palloc(sizeof(struct aod_agg_state));
+ stateptr->type = MUTABLE_SKETCH;
+ stateptr->lg_k = PG_NARGS() > 3 ? PG_GETARG_INT32(3) : 0;
+ stateptr->num_values = arr_len;
p = PG_NARGS() > 4 ? PG_GETARG_FLOAT4(4) : 1;
- if (lg_k) {
- sketchptr = p ? aod_sketch_new_lgk_p(arr_len, lg_k, p) : aod_sketch_new_lgk(arr_len, lg_k);
+ if (stateptr->lg_k) {
+ stateptr->ptr = p ? aod_sketch_new_lgk_p(arr_len, stateptr->lg_k, p) : aod_sketch_new_lgk(arr_len, stateptr->lg_k);
Review Comment:
I think this will always take the first branch unless p is explicitly set to 0?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1177475329
##########
src/theta_sketch_pg_functions.c:
##########
@@ -287,47 +343,98 @@ Datum pg_theta_union_get_result(PG_FUNCTION_ARGS) {
if (PG_ARGISNULL(0)) PG_RETURN_NULL();
if (!AggCheckCallContext(fcinfo, &aggcontext)) {
- elog(ERROR, "theta_union_get_result called in non-aggregate context");
+ elog(ERROR, "theta_sketch_serialize_state called in non-aggregate context");
}
oldcontext = MemoryContextSwitchTo(aggcontext);
- unionptr = PG_GETARG_POINTER(0);
- sketchptr = theta_union_get_result(unionptr);
- bytes_out = theta_sketch_serialize(sketchptr, VARHDRSZ);
- theta_sketch_delete(sketchptr);
- theta_union_delete(unionptr);
+ stateptr = (struct agg_state*) PG_GETARG_POINTER(0);
+ if (stateptr->type == MUTABLE_SKETCH) {
+ stateptr->ptr = theta_sketch_compact(stateptr->ptr);
Review Comment:
"how is a compact sketch mutable?" -- had to look at `theta_sketch_compact()` separately but that answered it
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 #63: Partial aggregation support
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov merged PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1177122042
##########
src/cpc_sketch_pg_functions.c:
##########
@@ -286,7 +368,6 @@ Datum pg_cpc_sketch_union(PG_FUNCTION_ARGS) {
cpc_sketch_delete(sketchptr2);
}
sketchptr = cpc_union_get_result(unionptr);
- cpc_union_delete(unionptr);
Review Comment:
Unclear why this was removed? It'll be cleaned up at the end of the transaction, but it seems to have been a temporary local variable?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1177130869
##########
src/cpc_sketch_pg_functions.c:
##########
@@ -286,7 +368,6 @@ Datum pg_cpc_sketch_union(PG_FUNCTION_ARGS) {
cpc_sketch_delete(sketchptr2);
}
sketchptr = cpc_union_get_result(unionptr);
- cpc_union_delete(unionptr);
Review Comment:
now get_result deletes the union
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 #63: Partial aggregation support
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#issuecomment-1520989827
I prepared the update script from 1.5.0 to 1.6.0 and added to this pull request since it is still open.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1176847839
##########
sql/datasketches--1.5.0--1.6.0.sql:
##########
@@ -0,0 +1,1350 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+CREATE OR REPLACE FUNCTION cpc_sketch_in(cstring) RETURNS cpc_sketch
+ AS '$libdir/datasketches', 'pg_sketch_in'
+ LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_out(cpc_sketch) RETURNS cstring
+ AS '$libdir/datasketches', 'pg_sketch_out'
+ LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_build_agg(internal, anyelement) RETURNS internal
+ AS '$libdir/datasketches', 'pg_cpc_sketch_build_agg'
+ LANGUAGE C IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_build_agg(internal, anyelement, int) RETURNS internal
+ AS '$libdir/datasketches', 'pg_cpc_sketch_build_agg'
+ LANGUAGE C IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_from_internal(internal) RETURNS cpc_sketch
Review Comment:
based on kll, i expected a change to serialize?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "AlexanderSaydakov (via GitHub)" <gi...@apache.org>.
AlexanderSaydakov commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1176946340
##########
src/aod_sketch_pg_functions.c:
##########
@@ -118,89 +125,43 @@ Datum pg_aod_sketch_add_item(PG_FUNCTION_ARGS) {
}
if (PG_ARGISNULL(0)) {
- lg_k = PG_NARGS() > 3 ? PG_GETARG_INT32(3) : 0;
+ stateptr = palloc(sizeof(struct aod_agg_state));
+ stateptr->type = MUTABLE_SKETCH;
+ stateptr->lg_k = PG_NARGS() > 3 ? PG_GETARG_INT32(3) : 0;
+ stateptr->num_values = arr_len;
p = PG_NARGS() > 4 ? PG_GETARG_FLOAT4(4) : 1;
- if (lg_k) {
- sketchptr = p ? aod_sketch_new_lgk_p(arr_len, lg_k, p) : aod_sketch_new_lgk(arr_len, lg_k);
+ if (stateptr->lg_k) {
+ stateptr->ptr = p ? aod_sketch_new_lgk_p(arr_len, stateptr->lg_k, p) : aod_sketch_new_lgk(arr_len, stateptr->lg_k);
Review Comment:
yeah, I think this is a mistake, but harmless. I will fix
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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 diff in pull request #63: Partial aggregation support
Posted by "jmalkin (via GitHub)" <gi...@apache.org>.
jmalkin commented on code in PR #63:
URL: https://github.com/apache/datasketches-postgresql/pull/63#discussion_r1176862723
##########
sql/datasketches--1.5.0--1.6.0.sql:
##########
@@ -0,0 +1,1350 @@
+-- Licensed to the Apache Software Foundation (ASF) under one
+-- or more contributor license agreements. See the NOTICE file
+-- distributed with this work for additional information
+-- regarding copyright ownership. The ASF licenses this file
+-- to you under the Apache License, Version 2.0 (the
+-- "License"); you may not use this file except in compliance
+-- with the License. You may obtain a copy of the License at
+--
+-- http://www.apache.org/licenses/LICENSE-2.0
+--
+-- Unless required by applicable law or agreed to in writing,
+-- software distributed under the License is distributed on an
+-- "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+-- KIND, either express or implied. See the License for the
+-- specific language governing permissions and limitations
+-- under the License.
+
+CREATE OR REPLACE FUNCTION cpc_sketch_in(cstring) RETURNS cpc_sketch
+ AS '$libdir/datasketches', 'pg_sketch_in'
+ LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_out(cpc_sketch) RETURNS cstring
+ AS '$libdir/datasketches', 'pg_sketch_out'
+ LANGUAGE C STRICT IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_build_agg(internal, anyelement) RETURNS internal
+ AS '$libdir/datasketches', 'pg_cpc_sketch_build_agg'
+ LANGUAGE C IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_build_agg(internal, anyelement, int) RETURNS internal
+ AS '$libdir/datasketches', 'pg_cpc_sketch_build_agg'
+ LANGUAGE C IMMUTABLE PARALLEL SAFE;
+
+CREATE OR REPLACE FUNCTION cpc_sketch_from_internal(internal) RETURNS cpc_sketch
Review Comment:
Nevermind, it's pretty clearly something around state management for distinct counting in particular. I'm sure it'll make more sense when I review the specific code around the implementation.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@datasketches.apache.org
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