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

For nested joins, there was an issue where calls to the inner most so… #211

Merged
merged 6 commits into from
Oct 24, 2018

Conversation

quinnj
Copy link
Contributor

@quinnj quinnj commented Oct 18, 2018

…urce got translated like :(a.(b.c)) while proper access should look like :((a.b).c). This commit adds an invert_getproperty_access translate function that performs the translation.

Fixes a query like:

using Query, DataFrames
mf = (senID=[1,2,3], leftNeighbor=[1,2,3], linearPosition=[1,2,3], word=["as", "the", "sailor"], simplecat=[1,2,3])
@from i in mf begin
    @join i11 in mf on (i.senID, i.leftNeighbor) equals (i11.senID, i11.linearPosition)
    @join i12 in mf on (i11.senID, i11.leftNeighbor) equals (i12.senID, i12.linearPosition)
    @join i13 in mf on (i12.senID, i12.leftNeighbor) equals (i13.senID, i13.linearPosition)
    @where i13.word == "as"
    @group i by i.simplecat into g
    @select {Key = key(g), Count = length(g)}
    @collect DataFrame
end

…urce got translated like :(a.(b.c)) while proper access should look like :((a.b).c). This commit adds an invert_getproperty_access translate function that performs the translation
@codecov-io
Copy link

codecov-io commented Oct 18, 2018

Codecov Report

Merging #211 into master will decrease coverage by 1.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #211      +/-   ##
=========================================
- Coverage   92.22%   91.2%   -1.03%     
=========================================
  Files           5       5              
  Lines         296     307      +11     
=========================================
+ Hits          273     280       +7     
- Misses         23      27       +4
Impacted Files Coverage Δ
src/query_translation.jl 91.45% <66.66%> (-1.14%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7900bbc...6222079. Read the comment docs.

@quinnj
Copy link
Contributor Author

quinnj commented Oct 19, 2018

@davidanthoff, this fixes the original queries Irene shared on slack (both queries with 3 joins & 5 joins). It seems to work on a handful of others I tried as well. The existing test suite also passes, so I don't think it breaks anything else. I added a few simple unit tests for the shift_access! function. It sounded on slack that there might be other issues this also helps? Maybe some of those queries can all be pooled together to add some more tests. Feel free to push/update this branch if you'd like.

@davidanthoff
Copy link
Member

This is fantastic, it also fixes #133! And many bug reports I got via other channels that I never even filed here because I always thought I had to fix the inference stuff to sort this out :)

I added one test with multiple @let statements that I think is the simplest case that gets at the core of this issue.

You mentioned some unit tests for shift_access!, but I think you might not have pushed them? Do you want to add those, and then I'll merge?

@quinnj
Copy link
Contributor Author

quinnj commented Oct 24, 2018

Ah, thanks for the reminder. I thought I had pushed the tests, but they were just sitting here locally. I just pushed them.

@davidanthoff
Copy link
Member

Cool. I'll wait for CI to pass and then I'll merge.

@davidanthoff davidanthoff merged commit 6990477 into queryverse:master Oct 24, 2018
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.

3 participants