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

BUG: Fix equality #1600

Closed
wants to merge 7 commits into from

Conversation

Projects
2 participants
@cpcloud
Copy link
Member

commented Aug 16, 2018

Closes #1599

This is a temporary fix that preserves most existing behavior until we can
refactor the compiler to separate compilation from optimization.

@cpcloud cpcloud modified the milestones: 0.14, 0.15 Aug 16, 2018

cpcloud added some commits Aug 14, 2018

@cpcloud cpcloud force-pushed the cpcloud:fix-equality branch from 035895b to 9334c23 Sep 5, 2018

cpcloud added some commits Sep 5, 2018

@cpcloud cpcloud changed the title WIP/BUG: Fix equality BUG: Fix equality Sep 5, 2018

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Sep 5, 2018

@kszucs can you review this PR when you get a chance?

@cpcloud cpcloud added this to To do in Refactoring via automation Sep 5, 2018

if self.equals(other):
return True

fn = lambda e: (lin.proceed, e.op()) # noqa: E731

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 5, 2018

Member

Perhaps It'd be useful to factor out this block to a lin.flatten function. I start to like even more this lin.traverse :)

This comment has been minimized.

Copy link
@cpcloud

cpcloud Sep 5, 2018

Author Member

Cool, can you open an issue for that?

This comment has been minimized.

Copy link
@kszucs
),
t2 AS (
)
SELECT t2.*

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 5, 2018

Member

Reviewing SQL string is surprisingly hard, but it looks way better.

FROM t0
WHERE t0.`o_totalprice` > (
SELECT avg(t7.`o_totalprice`) AS `mean`
FROM t0 t7

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 5, 2018

Member

At first it looks odd. If I'm correct t0 is tpch and t7 is tpch.view(), so it's actually correct.

@kszucs

kszucs approved these changes Sep 5, 2018

Copy link
Member

left a comment

It looks good!

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

@cpcloud Should I merge?

if hasattr(left, 'equals'):
return left.equals(right, cache=cache)
return left == right
return (hasattr(left, 'equals') and

This comment has been minimized.

Copy link
@kszucs

kszucs Sep 5, 2018

Member

IMO the previous was more readable.

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

The tests broke.

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Yeah, using and was incorrect because left == right would be executed regardless. Fixed

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Merging on green.

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

@cpcloud I've recently updated the clickhouse-*-feedstocks to support python 3.7 and one of the builds was broken. Theoretically it's enough to restart the build, however I cannot do that can You give me permissions (just seen it's your circleci)?

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

@kszucs Is this why the python35_test job failed? I can restart once the other builds finish

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Ok, it's still failing. I need to leave the openoffice now, I'll be in front of keyboard again in 30m..

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

Re. I'm reproducing it locally, now that I have bandwith.

@kszucs

This comment has been minimized.

Copy link
Member

commented Sep 6, 2018

@cpcloud Please restart your builds, should(!!!) work now!

@cpcloud

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

@kszucs Excellent, thanks for digging in.

@cpcloud cpcloud closed this in 591c058 Sep 6, 2018

Refactoring automation moved this from To do to Done Sep 6, 2018

@cpcloud cpcloud deleted the cpcloud:fix-equality branch Sep 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.