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: [OmniSciDB] Fix TopK when used as filter #2134

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

xmnlab
Copy link
Contributor

@xmnlab xmnlab commented Mar 11, 2020

Resolve #1918 . Additionally fix #2129

@xmnlab xmnlab added the bug Incorrect behavior inside of ibis label Mar 12, 2020
@@ -180,6 +180,7 @@ class OmniSciDBTableSetFormatter(compiles.TableSetFormatter):
_join_names = {
ops.InnerJoin: 'JOIN',
ops.LeftJoin: 'LEFT JOIN',
ops.LeftSemiJoin: 'JOIN', # needed by topk as filter
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if it is the best approach ... but at least it is working now.
maybe #512 would be a better approach to fix this issue

Copy link
Contributor

Choose a reason for hiding this comment

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

that's a really old issue, has things changed in the interim?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it seems it was not changed. but maybe we need to check this better.

Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

looks fine. can you add a release note and a question.

def table(self, name, path):
def table(
self, name: str, path: Optional[str] = None
) -> ibis.expr.types.TableExpr:
Copy link
Contributor

Choose a reason for hiding this comment

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

use the standard ibis import

import ibis.expr.types as ir (at the top)

@xmnlab
Copy link
Contributor Author

xmnlab commented Mar 12, 2020

release note added and ci green.

@jreback jreback modified the milestones: Next Feature Release, Next Bugfix Release Mar 14, 2020
@jreback
Copy link
Contributor

jreback commented Mar 14, 2020

lgtm. tip on release notes; don't put them at the top, rather insert between others so to avoid conflicts.

@jreback jreback merged commit da7a94a into ibis-project:master Mar 14, 2020
@jreback
Copy link
Contributor

jreback commented Mar 14, 2020

thanks @xmnlab

@xmnlab xmnlab deleted the omniscidb-bug-topk-join branch March 14, 2020 23:06
@xmnlab
Copy link
Contributor Author

xmnlab commented Mar 14, 2020

thanks @jreback for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis
Projects
None yet
2 participants