Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve catalog function interface #4733

Merged
merged 2 commits into from
Jan 18, 2025
Merged

Improve catalog function interface #4733

merged 2 commits into from
Jan 18, 2025

Conversation

andyfengHKU
Copy link
Contributor

Description

This is a followed-up PR of #4726. We now only expose FunctionEntry from catalog and hide FunctionCatalogSet. The same thing should be done for Macro but I kinda think it should first be moved as a separate CatalogSet. Right now in FunctionCatalogSet we have both FunctionCatalogEntry & MacroCatalogEntry. Either they should be the same CatalogEntry or they belong to different CatalogSet.

@acquamarin & @ray6080 we should discuss this.

Also @acquamarin can u test what will happen if we start doing

CALL macro_function() RETURN *
CALL scalar_function() RETURN *
RETURN table/gds_function()

I suspect we don't throw properly in some of these edge cases.

Fixes # (issue)

Contributor agreement

acquamarin
acquamarin approved these changes Jan 18, 2025
View reviewed changes
src/catalog/catalog.cpp
@@ -358,29 +363,19 @@ void Catalog::addFunction(Transaction* transaction, CatalogEntryType entryType,
}

void Catalog::dropFunction(Transaction* transaction, const std::string& name) {
const auto entry = functions->getEntry(transaction, name);
if (entry == nullptr) {
if (!containsFunction(transaction, name)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should binder always call containsFunction first before calling dropFunction?

@andyfengHKU andyfengHKU force-pushed the improve-catalog-function-interface branch from 55b864a to 81b7301 Compare January 18, 2025 04:05
@acquamarin
Copy link
Collaborator

I tested the following:

  1. CALL macro function
CREATE MACRO add5(x) AS x + 5;
CALL ADD5(3) RETURN *;
  1. CALL scalar function
CALL SIN(5) RETURN * => Error: `Binder exception: sin is not a table or algorithm function.`
  1. RETURN table function
return show_tables(); => Error: Binder exception: SHOW_TABLES is a TABLE_FUNCTION_ENTRY. Scalar function, aggregate function or macro was expected.

@github-actions GitHub Actions
Copy link

Benchmark Result

Master commit hash: bb6d3ec271ffb8e3ca8f7c59f3a873eae2385d77
Branch commit hash: f6d4e715ed2daac5e3ab64c2d441776f6114b89a

Query Group Query Name Mean Time - Commit (ms) Mean Time - Master (ms) Diff
aggregation q24 658.61 648.55 10.05 (1.55%)
aggregation q28 11435.23 12080.39 -645.17 (-5.34%)
filter q14 134.44 137.75 -3.30 (-2.40%)
filter q15 133.27 135.63 -2.36 (-1.74%)
filter q16 308.01 317.25 -9.24 (-2.91%)
filter q17 457.28 461.54 -4.26 (-0.92%)
filter q18 1940.65 1976.29 -35.64 (-1.80%)
filter zonemap-node 98.23 99.26 -1.02 (-1.03%)
filter zonemap-node-lhs-cast 98.64 100.23 -1.60 (-1.59%)
filter zonemap-node-null 93.92 96.17 -2.25 (-2.34%)
filter zonemap-rel 5748.28 5826.81 -78.53 (-1.35%)
fixed_size_expr_evaluator q07 580.49 580.11 0.38 (0.07%)
fixed_size_expr_evaluator q08 810.73 809.10 1.64 (0.20%)
fixed_size_expr_evaluator q09 814.39 809.33 5.06 (0.63%)
fixed_size_expr_evaluator q10 244.71 245.14 -0.43 (-0.18%)
fixed_size_expr_evaluator q11 241.83 237.50 4.33 (1.82%)
fixed_size_expr_evaluator q12 234.47 234.88 -0.41 (-0.17%)
fixed_size_expr_evaluator q13 1465.58 1465.19 0.39 (0.03%)
fixed_size_seq_scan q23 123.36 117.53 5.83 (4.96%)
join q29 628.46 591.63 36.83 (6.23%)
join q30 9447.40 9880.31 -432.91 (-4.38%)
join q31 7.83 6.95 0.88 (12.62%)
join SelectiveTwoHopJoin 54.78 58.37 -3.59 (-6.16%)
ldbc_snb_ic q35 2552.41 2606.92 -54.51 (-2.09%)
ldbc_snb_ic q36 469.71 463.38 6.32 (1.36%)
ldbc_snb_is q32 6.68 6.48 0.20 (3.15%)
ldbc_snb_is q33 12.91 15.61 -2.70 (-17.27%)
ldbc_snb_is q34 1.47 1.33 0.14 (10.51%)
multi-rel multi-rel-large-scan 1666.33 1604.33 62.00 (3.86%)
multi-rel multi-rel-lookup 53.77 43.77 10.00 (22.85%)
multi-rel multi-rel-small-scan 1433.13 1449.06 -15.93 (-1.10%)
order_by q25 140.84 136.69 4.15 (3.04%)
order_by q26 467.07 465.68 1.39 (0.30%)
order_by q27 1489.00 1479.21 9.79 (0.66%)
recursive_join recursive-join-bidirection 301.37 312.94 -11.58 (-3.70%)
recursive_join recursive-join-dense 7399.28 7395.65 3.63 (0.05%)
recursive_join recursive-join-path 23521.32 23451.36 69.96 (0.30%)
recursive_join recursive-join-sparse 1064.11 1066.37 -2.26 (-0.21%)
recursive_join recursive-join-trail 7375.20 7348.22 26.98 (0.37%)
scan_after_filter q01 178.22 178.91 -0.69 (-0.38%)
scan_after_filter q02 165.78 167.34 -1.56 (-0.93%)
shortest_path_ldbc100 q37 84.75 89.08 -4.33 (-4.86%)
shortest_path_ldbc100 q38 350.54 355.57 -5.03 (-1.42%)
shortest_path_ldbc100 q39 65.56 63.84 1.72 (2.69%)
shortest_path_ldbc100 q40 383.85 371.90 11.95 (3.21%)
var_size_expr_evaluator q03 2179.95 2087.31 92.64 (4.44%)
var_size_expr_evaluator q04 2260.63 2269.74 -9.11 (-0.40%)
var_size_expr_evaluator q05 2679.40 5299.71 -2620.30 (-49.44%)
var_size_expr_evaluator q06 1379.17 1332.67 46.50 (3.49%)
var_size_seq_scan q19 1488.03 1455.58 32.45 (2.23%)
var_size_seq_scan q20 2696.84 2669.81 27.03 (1.01%)
var_size_seq_scan q21 2308.16 2310.84 -2.68 (-0.12%)
var_size_seq_scan q22 130.73 128.30 2.43 (1.90%)

@andyfengHKU andyfengHKU force-pushed the improve-catalog-function-interface branch from 277cdee to 631d7be Compare January 18, 2025 05:39
@github-actions GitHub Actions
Copy link

Benchmark Result

Master commit hash: ec36a22f5a408f81b18bde36dae37c83ae3fe4b0
Branch commit hash: 5d679684074843598948a5fa6da7ff8331a98500

Query Group Query Name Mean Time - Commit (ms) Mean Time - Master (ms) Diff
aggregation q24 647.48 639.99 7.49 (1.17%)
aggregation q28 11620.89 11095.37 525.52 (4.74%)
filter q14 137.54 128.55 8.99 (6.99%)
filter q15 134.78 127.78 6.99 (5.47%)
filter q16 309.90 300.98 8.91 (2.96%)
filter q17 457.69 454.67 3.01 (0.66%)
filter q18 1957.00 2002.49 -45.49 (-2.27%)
filter zonemap-node 98.15 89.31 8.84 (9.90%)
filter zonemap-node-lhs-cast 96.93 89.56 7.37 (8.23%)
filter zonemap-node-null 93.45 87.16 6.29 (7.22%)
filter zonemap-rel 5758.28 5884.88 -126.60 (-2.15%)
fixed_size_expr_evaluator q07 580.28 575.33 4.95 (0.86%)
fixed_size_expr_evaluator q08 812.82 811.46 1.36 (0.17%)
fixed_size_expr_evaluator q09 811.99 807.97 4.02 (0.50%)
fixed_size_expr_evaluator q10 246.50 236.99 9.51 (4.01%)
fixed_size_expr_evaluator q11 239.96 229.87 10.08 (4.39%)
fixed_size_expr_evaluator q12 235.11 226.57 8.54 (3.77%)
fixed_size_expr_evaluator q13 1477.01 1459.75 17.26 (1.18%)
fixed_size_seq_scan q23 125.27 111.53 13.74 (12.32%)
join q29 588.59 621.56 -32.97 (-5.31%)
join q30 9999.59 10001.75 -2.16 (-0.02%)
join q31 4.94 5.80 -0.86 (-14.84%)
join SelectiveTwoHopJoin 50.72 54.21 -3.49 (-6.44%)
ldbc_snb_ic q35 2513.41 2599.80 -86.39 (-3.32%)
ldbc_snb_ic q36 476.23 474.50 1.73 (0.36%)
ldbc_snb_is q32 6.85 4.17 2.68 (64.35%)
ldbc_snb_is q33 14.99 12.63 2.36 (18.70%)
ldbc_snb_is q34 1.35 1.37 -0.02 (-1.41%)
multi-rel multi-rel-large-scan 1637.56 1537.35 100.21 (6.52%)
multi-rel multi-rel-lookup 72.04 43.32 28.72 (66.29%)
multi-rel multi-rel-small-scan 1459.67 1405.31 54.36 (3.87%)
order_by q25 144.91 133.17 11.75 (8.82%)
order_by q26 460.69 451.96 8.73 (1.93%)
order_by q27 1501.04 1449.47 51.57 (3.56%)
recursive_join recursive-join-bidirection 271.46 276.76 -5.30 (-1.91%)
recursive_join recursive-join-dense 5076.72 7382.04 -2305.32 (-31.23%)
recursive_join recursive-join-path 22967.58 23812.05 -844.47 (-3.55%)
recursive_join recursive-join-sparse 1061.71 1066.34 -4.63 (-0.43%)
recursive_join recursive-join-trail 5775.99 7328.31 -1552.32 (-21.18%)
scan_after_filter q01 181.53 171.62 9.91 (5.78%)
scan_after_filter q02 167.14 157.56 9.58 (6.08%)
shortest_path_ldbc100 q37 93.15 104.31 -11.15 (-10.69%)
shortest_path_ldbc100 q38 371.20 374.30 -3.10 (-0.83%)
shortest_path_ldbc100 q39 61.99 60.16 1.83 (3.04%)
shortest_path_ldbc100 q40 448.83 459.82 -10.98 (-2.39%)
var_size_expr_evaluator q03 2096.56 2067.65 28.91 (1.40%)
var_size_expr_evaluator q04 2239.30 2229.37 9.92 (0.45%)
var_size_expr_evaluator q05 2626.29 2600.95 25.34 (0.97%)
var_size_expr_evaluator q06 1357.57 1321.60 35.97 (2.72%)
var_size_seq_scan q19 1479.84 1456.56 23.28 (1.60%)
var_size_seq_scan q20 2670.08 2777.64 -107.56 (-3.87%)
var_size_seq_scan q21 2291.30 2296.52 -5.22 (-0.23%)
var_size_seq_scan q22 131.22 126.61 4.61 (3.64%)

@andyfengHKU andyfengHKU merged commit 066b3d5 into master Jan 18, 2025
@andyfengHKU andyfengHKU deleted the improve-catalog-function-interface branch January 18, 2025 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ray6080 ray6080 ray6080 approved these changes

@acquamarin acquamarin acquamarin approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andyfengHKU @acquamarin @ray6080