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.

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

Add SQL aggregates ANY, SOME, EVERY and their COLL_ versions #360

Merged
merged 4 commits into from
Aug 9, 2023

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented May 8, 2023

Adds the SQL aggregate functions ANY, SOME, and EVERY along with their COLL_ versions (i.e. COLL_ANY, COLL_SOME, COLL_EVERY).

This PR also

  • fixes a slight bug related to the previously implemented SQL aggregates when an aggregation result is NULL or MISSING.
  • closes Implement Strict mode errors for COLL_* functions #419 with some fixes to the strict mode type checking behavior of the COLL_ functions.
  • adds a check to logical plan lowering for HAVING without GROUP BY

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@alancai98 alancai98 self-assigned this May 8, 2023
@codecov
Copy link

codecov bot commented May 8, 2023

Codecov Report

Patch coverage: 85.59% and project coverage change: +0.04% 🎉

Comparison is base (191b10e) 81.70% compared to head (d5677cb) 81.74%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
+ Coverage   81.70%   81.74%   +0.04%     
==========================================
  Files          62       62              
  Lines       15235    15426     +191     
  Branches    15235    15426     +191     
==========================================
+ Hits        12447    12610     +163     
- Misses       2285     2303      +18     
- Partials      503      513      +10     
Files Changed Coverage Δ
partiql-ast-passes/src/error.rs 62.50% <ø> (ø)
partiql-logical/src/lib.rs 40.46% <ø> (ø)
partiql-parser/src/parse/parser_state.rs 100.00% <ø> (ø)
partiql-types/src/lib.rs 68.75% <ø> (-0.63%) ⬇️
partiql-logical-planner/src/builtins.rs 95.64% <65.71%> (-3.16%) ⬇️
partiql-eval/src/eval/evaluable.rs 90.22% <88.73%> (+0.81%) ⬆️
partiql-eval/src/eval/expr/coll.rs 94.54% <96.36%> (-2.92%) ⬇️
partiql-eval/src/eval/eval_expr_wrapper.rs 85.15% <100.00%> (+0.89%) ⬆️
partiql-eval/src/plan.rs 96.79% <100.00%> (+0.23%) ⬆️
partiql-logical-planner/src/lower.rs 85.81% <100.00%> (+0.08%) ⬆️
... and 1 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions
Copy link

github-actions bot commented May 8, 2023

Conformance comparison report

Base (191b10e) 2ee13a1 +/-
% Passing 86.33% 88.46% 2.13%
✅ Passing 5458 5595 137
❌ Failing 864 730 -134
🔶 Ignored 0 0 0
Total Tests 6322 6325 3

Number passing in both: 5455

Number failing in both: 729

Number passing in Base (191b10e) but now fail: 0

Number failing in Base (191b10e) but now pass: 132

The following test(s) were previously failing but now pass. Before merging, confirm they are intended to pass:

Click here to see
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_null_and_missing_only
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_bag_literals
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_non_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_one_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_null
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_missing
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_avg::strict_coll_avg_mistyped_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_single_true
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_every::coll_every_all_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_single_true
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_some::sql_some_distinct_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_single_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_bag_of_missing_elements
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_any::coll_any_all_aggregate_function_call
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_every::coll_every_distinct_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_null
  • partiql_tests::eval_equiv::spec_tests::section_5::strict_equiv_of_comma_cross_join_and_join
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_any::sql_any_distinct_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_some_empty
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_single_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_bag_of_missing_elements
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_any::coll_any_distinct_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_nulls_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_single_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_empty_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_missing
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_non_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_single_false
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_some::coll_some_distinct_aggregate_function_call
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_some::permissive_some_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_nulls_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_single_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_missing
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_bag_of_missing_elements
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_some::sql_some_all_aggregate_function_call
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_every::permissive_every_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_nulls_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_missing
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_every::permissive_every_distinct_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_nulls_only
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_one_non_bool_non_unknown
  • partiql_tests::eval_equiv::spec_tests::section_5::permissive_equiv_of_comma_cross_join_and_join
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_missing
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_nested_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_null_and_missing_with_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_one_non_bool_non_unknown
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::permissive_limit_offset_after_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_non_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_bag_literals
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_null_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_non_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_nulls_with_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_bag_of_missing_elements
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_bag_of_missing_elements
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_list_expressions
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_some::strict_some_distinct_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_list_expressions
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_list_expressions
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_nested_collection
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::permissive_limit_2_offset_2
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_bag_literals
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_null
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_some::permissive_some_distinct_with_group_by
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::permissive_offset_group_by_having
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_list_expressions
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_empty_collection
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_any::permissive_any_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_bag_literals
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_non_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_nulls_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_null_and_missing_only
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_every::sql_every_distinct_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_non_collection
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_any::sql_any_all_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_some_empty
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_null_with_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_single_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_nested_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_sum::strict_coll_sum_mistyped_element
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::strict_limit_2_offset_2
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_null_and_missing_with_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_single_false
  • partiql_tests::success::syntax::primitives::primitives::coll_aggregate_function::coll_some::coll_some_all_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_null
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_nested_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_single_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_one_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_null
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_nulls_with_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_bag_literals
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_any::permissive_any_distinct_with_group_by
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_every::strict_every_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_nulls_only
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_any::strict_any_with_group_by
  • partiql_tests::eval::query::select::select::select::select_bugs::permissive_mysql_select_bug_02
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_one_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_some_empty
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_list_of_missing_element
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_missing
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_list_expressions
  • partiql_tests::success::syntax::query::query::select::sql_aggregate::sql_every::sql_every_all_aggregate_function_call
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_some_empty
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_one_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_list_expressions
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_every::strict_every_distinct_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_nulls_only
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_null
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_nested_collection
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::permissive_coll_any_bag_of_missing_elements
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_nested_collection
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::strict_offset_group_by_having
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_some::strict_some_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_bag_literals
  • partiql_tests::eval::query::select::select::sql_aggregate::sql_any::strict_any_distinct_with_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::permissive_coll_every_all_non_bool_non_unknown
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_single_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_any::strict_coll_any_nulls_with_true
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_every::strict_coll_every_single_false
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::permissive_coll_some_nulls_only
  • partiql_tests::eval::query::limitoffset::limitoffset::limit_offset::strict_limit_offset_after_group_by
  • partiql_tests::eval::primitives::coll_aggregate_function::coll_aggregate_function::coll_some::strict_coll_some_nulls_with_true
  • partiql_tests::eval::query::select::select::select::select_bugs::strict_mysql_select_bug_02

@alancai98 alancai98 requested review from jpschorr and am357 May 8, 2023 23:06
Base automatically changed from add-coll-fns to main May 25, 2023 02:59
@alancai98 alancai98 marked this pull request as draft August 2, 2023 00:19
@alancai98
Copy link
Member Author

Marking PR as draft. Will rebase on #413 once that PR is merged in to account for evaluation function binding changes.

@alancai98
Copy link
Member Author

alancai98 commented Aug 4, 2023

Test regression is partially due to a test misclassification that is corrected in: partiql/partiql-tests#107.

Basically, the test (SELECT rep, SUM(total_sales) as total FROM sales_report HAVING rep = \"Meg\") expects an evaluation error, but was previously giving the wrong error. It was previously giving an evaluation error that the aggregation was empty when the error should be that a HAVING clause was provided without a GROUP BY. This error should be caught during lowering, hence the conformance test PR to reclassify the error as a StaticAnalysisFail rather than EvaluationFail.

Once that conformance test PR is merged in and I update this PR's partiql-tests submodule, the tests should pass.

@alancai98 alancai98 marked this pull request as ready for review August 4, 2023 23:38
Comment on lines +711 to +716
if select.having.is_some() && select.group_by.is_none() {
self.errors.push(AstTransformError::HavingWithoutGroupBy);
Traverse::Stop
} else {
Traverse::Continue
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Some context for why this change is in this PR provided in this comment: #360 (comment)

@alancai98 alancai98 force-pushed the add-any-some-every branch 2 times, most recently from 7b818ad to 751b5ce Compare August 4, 2023 23:51
@alancai98 alancai98 merged commit 59a5fb1 into main Aug 9, 2023
19 checks passed
@alancai98 alancai98 deleted the add-any-some-every branch August 9, 2023 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Strict mode errors for COLL_* functions
3 participants