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

Extend Aggregate_Spec test suite with tests for missed edge-cases to ensure the feature is well-tested on all backends #3383

Merged
merged 26 commits into from
Apr 12, 2022
Merged

Extend Aggregate_Spec test suite with tests for missed edge-cases to ensure the feature is well-tested on all backends #3383

merged 26 commits into from
Apr 12, 2022

Conversation

radeusgd
Copy link
Member

@radeusgd radeusgd commented Apr 6, 2022 •
edited
Loading

Pull Request Description

Implements https://www.pivotaltracker.com/story/show/181805693 and finishes the basic set of features of the Aggregate component.

Still not all aggregations are supported everywhere, because for example SQLite has quite limited support for aggregations. Currently the workaround is to bring the table into memory (if possible) and perform the computation locally. Later on, we may add more complex generator features to emulate the missing aggregations with complex sub-queries.

Important Notes

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 dist and ./run watch.

@radeusgd radeusgd self-assigned this Apr 6, 2022
@radeusgd radeusgd force-pushed the wip/radeusgd/aggregate-edge-case-tests-181805693 branch from 8cc25b9 to 490e7f1 Compare April 8, 2022 11:00
@radeusgd radeusgd marked this pull request as ready for review April 8, 2022 15:32
@radeusgd radeusgd force-pushed the wip/radeusgd/aggregate-edge-case-tests-181805693 branch 2 times, most recently from 9fa5e78 to 4fea088 Compare April 8, 2022 15:46
@radeusgd radeusgd requested a review from hubertp April 11, 2022 07:57
jdunkerley
jdunkerley requested changes Apr 11, 2022
View reviewed changes
Copy link
Member

@jdunkerley jdunkerley left a comment

Choose a reason for hiding this comment

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

Generally LGTM but a few little tweaks please

@radeusgd
Simplify table_builder
292f472
@radeusgd
Add skeleton for tests
1a6fa6c
@radeusgd
Add Concat tests
c5ef87f
@radeusgd
fixes for concat
30e4bc4
@radeusgd
Improve SQL error handling: add contextual query info.
1aa6009
@radeusgd
Fix Postgres, make pending status clearer; fix Sqlite spec
f7aed9b
@radeusgd
Add no agg edges
217847f
@radeusgd
Count_Distinct tests
d187af6 
WIP: Postgres not returning correct results ?
@radeusgd
Fix count distinct for Postgres
bf9d2a6
@radeusgd
Sort out SQLite
fe5d988
@radeusgd
Refactor text ops to be in the same format as others
1a43d0b
@radeusgd
Fix SQLite stddev
b841969
@radeusgd
FIgure out stddev
3ef2a11
@radeusgd
Add null special cases to not fail displaying recursive definition er…
3818401 
…rors

This is a temporary solution which may need to be reverted once a proper solution is developed.
@radeusgd
Some fixes to handling infinities in Percentile (and Median)
2e46e24
@radeusgd
NaN in In-Mem Median and Percentile
d2e550d
@radeusgd radeusgd force-pushed the wip/radeusgd/aggregate-edge-case-tests-181805693 branch from f627191 to 95df815 Compare April 11, 2022 13:14
@radeusgd radeusgd added Category: Libraries CI: Ready to merge This PR is eligible for automatic merge labels Apr 11, 2022
hubertp
hubertp approved these changes Apr 11, 2022
View reviewed changes
Copy link
Contributor

@hubertp hubertp left a comment

Choose a reason for hiding this comment

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

nits

distribution/lib/Standard/Base/0.0.0-dev/src/Data/Number/Extensions.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Database/0.0.0-dev/src/Connection/Connection.enso Show resolved Hide resolved
distribution/lib/Standard/Test/0.0.0-dev/src/Main.enso Outdated Show resolved Hide resolved
distribution/lib/Standard/Test/0.0.0-dev/src/Main.enso
case matches of
True -> Success
False ->
loc = Meta.get_source_location 2+frames_to_skip
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this was here before but where does the magic 2 come from?

Copy link
Member Author

Choose a reason for hiding this comment

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

The number of frames of how deep inside of should_equal we are. I think the case itself adds one frame and then the particular branch another - was experimentally measured to give the correct result. Maybe worth double-checking.

Ideally we should have tests for this - but currently we do not have a robust way to test the test suite using the test suite, I think :D

...e/runtime/src/main/java/org/enso/interpreter/node/expression/builtin/text/AnyToTextNode.java
@@ -64,7 +64,9 @@ private Text doComplexAtom(Atom atom) {

@CompilerDirectives.TruffleBoundary
private String showObject(Object child) throws UnsupportedMessageException {
if (child instanceof Boolean) {
if (child == null) {
return "null";
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "Nothing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question, I don't know. This is not really a Nothing instance but an unitialized value.
This relates to the recursive bug thread on Discord.

This is just a minimal workaround to avoid errors that are hard to debug when a null leaks into these methods (up to now, the showObject would fail when trying to display the original error so we'd get just a random NPE without any knowledge of the underlying issue, this fix allows us to see the issue more clearly).

This is not a good fix really - it's in no way comprehensive - there are still places where I think the null could leak - I just plumbed the ones I encountered. A proper fix was, like @kustosz suggested, adding null-checks at the reads, or if I understand correctly we could introduce some non-null sentinel value to which uninitialized data could be set when running its initialization. Then these particular checks will become redundant and should be removed.

So I guess I could add a TODO here with a link to https://www.pivotaltracker.com/story/show/181652974

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

std-bits/table/src/main/java/org/enso/table/aggregations/StandardDeviation.java Outdated Show resolved Hide resolved
@radeusgd radeusgd removed the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2022
@radeusgd radeusgd added the CI: Ready to merge This PR is eligible for automatic merge label Apr 11, 2022
@mergify mergify bot merged commit 891f064 into develop Apr 12, 2022
@mergify mergify bot deleted the wip/radeusgd/aggregate-edge-case-tests-181805693 branch April 12, 2022 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hubertp hubertp hubertp approved these changes

@4e6 4e6 4e6 approved these changes

@jdunkerley jdunkerley jdunkerley approved these changes

Assignees

@radeusgd radeusgd

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.

None yet
4 participants
@radeusgd @hubertp @4e6 @jdunkerley