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

Fixes to ORDER BY name resolution and null ordering bug #418

Merged
merged 25 commits into from
Aug 10, 2023

Conversation

alancai98
Copy link
Member

@alancai98 alancai98 commented Aug 2, 2023

(PR targets change-var-resolution branch. Once #416 is merged in, this PR will target main)

Fixes some previous bugs to get more ORDER BY tests to pass:

  • Some name resolution fixes. Previous dataflow issue remains related to select aliases referenced in order by
  • Null/missing ordering for nested collections (kudos to @jpschorr on the initial factoring there)
    • For some context, the previous Ord trait implementation always assumed NULLS FIRST. So even though the ORDER BY allowed configuring NULLS FIRST or NULLS LAST, nested collections would not sort according to the specified sort order. E.g. comparing [null] with [1] with NULLS LAST should return Ordering::Greater but instead returned Ordering::Less

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 Aug 2, 2023
@alancai98 alancai98 force-pushed the fix-order-by-name-resolution branch from a9e9e5b to d0fd94e Compare August 2, 2023 00:22
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

Conformance comparison report

Base (2cbc7a4) 447539d +/-
% Passing 88.40% 89.06% 0.66%
✅ Passing 5607 5649 42
❌ Failing 736 694 -42
🔶 Ignored 0 0 0
Total Tests 6343 6343 0

Number passing in both: 5607

Number failing in both: 694

Number passing in Base (2cbc7a4) but now fail: 0

Number failing in Base (2cbc7a4) but now pass: 42

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::query::order_by::order_by::edge_cases::strict_false_before_true_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_inf_before_numeric_values_then_inf_then_nan_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lob_types_should_ordered_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lob_types_follow_their_lexicographical_ordering_by_octet_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_shorter_array_comes_first_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lists_items_should_be_ordered_by_data_types_asc_nulls_last_as_default_for_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lists_compared_lexicographically_based_on_comparison_of_elements_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_structs_compared_lexicographically_first_by_key_then_by_value_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_text_types_compared_by_lexicographical_ordering_of_unicode_scalar_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_text_types_compared_by_lexicographical_ordering_of_unicode_scalar_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_longer_array_comes_first_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_nan_before_inf_then_numeric_values_then_inf_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_bags_compared_as_sorted_lists_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_text_types_compared_by_lexicographical_ordering_of_unicode_scalar_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_bags_compared_as_sorted_lists_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lob_types_should_ordered_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_structs_should_be_ordered_by_data_types_desc_nulls_first_as_default_for_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_structs_compared_lexicographically_first_by_key_then_by_value_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lists_items_should_be_ordered_by_data_types_desc_nulls_first_as_default_for_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lists_compared_lexicographically_based_on_comparison_of_elements_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lists_items_should_be_ordered_by_data_types_asc_nulls_last_as_default_for_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_longer_array_comes_first_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_structs_should_be_ordered_by_data_types_desc_nulls_first_as_default_for_desc
  • partiql_tests::eval::primitives::symbol::symbol::empty_symbol::strict_empty_symbol_in_table
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_shorter_array_comes_first_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_bags_compared_as_sorted_lists_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_structs_compared_lexicographically_first_by_key_then_by_value_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_inf_before_numeric_values_then_inf_then_nan_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lists_items_should_be_ordered_by_data_types_desc_nulls_first_as_default_for_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_true_before_false_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_true_before_false_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_bags_compared_as_sorted_lists_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lists_compared_lexicographically_based_on_comparison_of_elements_desc
  • partiql_tests::eval::primitives::symbol::symbol::empty_symbol::permissive_empty_symbol_in_table
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_false_before_true_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_structs_compared_lexicographically_first_by_key_then_by_value_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_structs_should_be_ordered_by_data_types_asc_nulls_last_as_default_for_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_lob_types_follow_their_lexicographical_ordering_by_octet_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::strict_structs_should_be_ordered_by_data_types_asc_nulls_last_as_default_for_asc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_text_types_compared_by_lexicographical_ordering_of_unicode_scalar_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_lists_compared_lexicographically_based_on_comparison_of_elements_desc
  • partiql_tests::eval::query::order_by::order_by::edge_cases::permissive_nan_before_inf_then_numeric_values_then_inf_asc

@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 94.89% and project coverage change: +0.23% 🎉

Comparison is base (2cbc7a4) 81.82% compared to head (327be8c) 82.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #418      +/-   ##
==========================================
+ Coverage   81.82%   82.05%   +0.23%     
==========================================
  Files          62       62              
  Lines       15548    15706     +158     
  Branches    15548    15706     +158     
==========================================
+ Hits        12722    12888     +166     
+ Misses       2307     2299       -8     
  Partials      519      519              
Files Changed Coverage Δ
partiql-value/src/tuple.rs 89.28% <88.88%> (-0.06%) ⬇️
partiql-logical-planner/src/lower.rs 86.39% <93.10%> (-0.02%) ⬇️
partiql-value/src/lib.rs 96.10% <94.73%> (+0.16%) ⬆️
partiql-eval/src/eval/evaluable.rs 91.00% <100.00%> (+0.78%) ⬆️
partiql-value/src/bag.rs 74.19% <100.00%> (+6.33%) ⬆️
partiql-value/src/list.rs 63.91% <100.00%> (+9.70%) ⬆️

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

@alancai98 alancai98 requested review from jpschorr and am357 August 3, 2023 00:32
@alancai98 alancai98 marked this pull request as ready for review August 3, 2023 00:32
@alancai98 alancai98 changed the title [WIP] Partially fix name resolution for ORDER BY Fixes to ORDER BY name resolution and null ordering configuration Aug 3, 2023
@alancai98 alancai98 changed the title Fixes to ORDER BY name resolution and null ordering configuration Fixes to ORDER BY name resolution and null ordering bug Aug 3, 2023
Comment on lines +1023 to +1030
let wrap = NullSortedValue::<false, Value>;
let (l, r) = (wrap(l.as_ref()), wrap(r.as_ref()));
r.cmp(&l)
}
EvalOrderBySortSpec::DescNullsLast => {
let wrap = NullSortedValue::<true, Value>;
let (l, r) = (wrap(l.as_ref()), wrap(r.as_ref()));
r.cmp(&l)
Copy link
Member Author

Choose a reason for hiding this comment

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

These are doing the correct ordering with respect to null/missing values.

For DescNullsFirst, it'll do the full ordering with nulls assigned to be Ordering::Greater (i.e. the false). Since we do r compared to l, the result is flipped, so this will order in descending order with the null/missing values first.

For DescNullsLast, it'll do the full ordering with nulls assigned to be Ordering::Less (i.e. the true). And since we do r compared to l, the result is flipped. This will order in descending order with the null/missing values last.

I noticed these were not part of the conformance tests, so added some tests in this PR: partiql/partiql-tests#106.

Base automatically changed from change-var-resolution to main August 10, 2023 18:13
@alancai98 alancai98 merged commit d93dfe7 into main Aug 10, 2023
@alancai98 alancai98 deleted the fix-order-by-name-resolution branch August 10, 2023 18:54
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.

2 participants