-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
pg_approx_hll #601
pg_approx_hll #601
Conversation
xref: |
manual testing result tested schema: cubejs create valid hll column included table when hit distinct apprx - preaggregation see valid DDL command DROP TABLE stb_pre_aggregations.orders__distinct_approx_3yrotyzx_gymlzk4i_1587213010940; CREATE TABLE stb_pre_aggregations.orders__distinct_approx_3yrotyzx_gymlzk4i_1587213010940 ALTER TABLE stb_pre_aggregations.orders__distinct_approx_3yrotyzx_gymlzk4i_1587213010940 then generate valid sql ` Executing SQL: 03601c10-804d-4a40-8c8b-f2894ddb5b21-span-1 COALESCE(ords.name,'Empty') as order_source_name, WHERE ol.ad_client_id=1000026 ) AS "orders" Performing query completed: 03601c10-804d-4a40-8c8b-f2894ddb5b21-span-1 (242ms) Executing Load Pre Aggregation SQL: 03601c10-804d-4a40-8c8b-f2894ddb5b21-span-1 CREATE TABLE stb_pre_aggregations.orders__distinct_approx_3yrotyzx_gymlzk4i_1587213010940 AS SELECT COALESCE(ords.name,'Empty') as order_source_name, WHERE ol.ad_client_id=1000026 ) AS "orders" Executing SQL: 03601c10-804d-4a40-8c8b-f2894ddb5b21-span-1 SELECT "orders__dateordered_day" "orders__dateordered_day", hll_cardinality(hll_union_agg("orders__order_distinct_approx")) "orders__order_dist Performing query completed: 03601c10-804d-4a40-8c8b-f2894ddb5b21-span-1 (147ms) { Load Request Success: d8a71e17-4965-43fc-bf71-b1abc7fdd6de-span-1 (6ms) |
|
||
hllMerge(sql) { | ||
return `hll_cardinality(hll_union_agg(${sql}))`; | ||
return sql; |
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.
I believe this one is redundant.
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.
will remove that 'return'
} | ||
|
||
countDistinctApprox(sql) { | ||
return `hll_cardinality(${sql})` |
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 seems incorrect. Could you please test it?
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 worked for our test schema. I will change it for hll_cardinality(hll_add_agg(hll_hash_any(${sql})))
.
Hey @milanbella Hey Milan! Overall looks great! Could you please check comments I provided. |
@milanbella Looks good! Thanks for contributing this! |
Check List
Issue Reference this PR resolves
[For example #12]
Description of Changes Made (if issue reference is not provided)
[Description goes here]