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

Implement In-Memory Table order_by #3515

Merged
merged 17 commits into from
Jun 8, 2022
Merged

Implement In-Memory Table order_by #3515

merged 17 commits into from
Jun 8, 2022

Conversation

jdunkerley
Copy link
Member

@jdunkerley jdunkerley commented Jun 7, 2022 •
edited
Loading

Pull Request Description

Implemented the order_by function with support for all modes of operation.
Added support for case insensitive natural order.

Important Notes

  • Improved MultiValueIndex/Key to not create loads of arrays.
  • Adjusted HashCode for MultiValueKey to have a simple algorithm.
  • Added Text_Utils.compare_normalized_ignoring_case to allow for case insensitive comparisons.
  • Fixed issues with ObjectComparator and added some unit tests for it.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide dist and ./run ide watch.

@jdunkerley jdunkerley force-pushed the wip/jd/table-order-by-182195322 branch from 9f8c0e7 to 509f3b1 Compare June 7, 2022 13:01
@jdunkerley jdunkerley marked this pull request as ready for review June 7, 2022 16:05
@jdunkerley jdunkerley force-pushed the wip/jd/table-order-by-182195322 branch from bb53225 to 972da1b Compare June 7, 2022 16:06
radeusgd
radeusgd reviewed Jun 8, 2022
View reviewed changes
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks generally good to me, some code suggestions inline.

One issue I'd like to discuss before merge is the equals/compareTo inconsistency in MultiValueKey, see below.

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering/Comparator.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Base/0.0.0-dev/src/Data/Ordering/Natural_Order.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Data/Dialect/Postgres.enso Show resolved Hide resolved
std-bits/base/src/main/java/org/enso/base/ObjectComparator.java Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/data/index/MultiValueIndex.java
case Storage.Type.LONG -> NumericBuilder.createLongBuilder(size);
case Storage.Type.STRING -> new StringBuilder(size);
case Storage.Type.OBJECT -> new ObjectBuilder(size);
default -> new InferredBuilder(size);
Copy link
Member

Choose a reason for hiding this comment

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

Is the default ever reachable?

Copy link
Member Author

Choose a reason for hiding this comment

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

in theory not as not an enumeration for Type wanted a safety fall back.
One to revisit when we handle column data types.

std-bits/table/src/main/java/org/enso/table/data/index/MultiValueKey.java Outdated Show resolved Hide resolved
std-bits/table/src/main/java/org/enso/table/data/index/MultiValueKey.java Outdated Show resolved Hide resolved
test/Tests/src/Data/Ordering/Comparator_Spec.enso
((default_comparator (Ord 1) (Ord 1)) == 0) . should_equal True

Test.specify "should fail gracefully for incomparable items" <|
(default_comparator 1 True).should_fail_with Vector.Incomparable_Values_Error
Copy link
Member

Choose a reason for hiding this comment

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

I think we should move the Incomparable_Values_Error away from Vector as it is used in many other places now.

@jdunkerley jdunkerley added the CI: Ready to merge This PR is eligible for automatic merge label Jun 8, 2022
radeusgd and others added 16 commits June 8, 2022 12:42
@jdunkerley jdunkerley force-pushed the wip/jd/table-order-by-182195322 branch from 3b67a27 to 96a7930 Compare June 8, 2022 11:43
@mergify mergify bot merged commit 8afba43 into develop Jun 8, 2022
@mergify mergify bot deleted the wip/jd/table-order-by-182195322 branch June 8, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@radeusgd radeusgd radeusgd approved these changes

@4e6 4e6 Awaiting requested review from 4e6

@hubertp hubertp Awaiting requested review from hubertp

@JaroslavTulach JaroslavTulach Awaiting requested review from JaroslavTulach

Assignees
No one assigned
Labels
CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jdunkerley @radeusgd