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

ENH/BUG: Fix broken decimal type support #1541

Closed
wants to merge 11 commits into from

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Jul 15, 2018

There are a number of places where we are computing the incorrect output_type for some operations as well as computing a float64 output type for operations whose inputs are decimals. Impala explicitly does this and so does BigQuery. The upshot of this is that when schema.apply_to(result) is called in these backends we will convert the data (which will be float64 Series objects) into Decimal columns.

@cpcloud cpcloud self-assigned this Jul 15, 2018
@cpcloud cpcloud added the expressions Issues or PRs related to the expression API label Jul 15, 2018
@cpcloud cpcloud added this to the 0.14 milestone Jul 15, 2018
@cpcloud cpcloud added the bug Incorrect behavior inside of ibis label Jul 15, 2018
@@ -9,7 +9,7 @@ RUN apt-get -qq update -y \
ARG PYTHON
ARG ENVKIND

ADD ci/requirements-${ENVKIND}-${PYTHON}.yml /
COPY ci/requirements-${ENVKIND}-${PYTHON}.yml /
Copy link
Member

Choose a reason for hiding this comment

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

kiss

dtype = arg.type()
else:
dtype = dt.double
return rlz.shape_like(arg, dtype)
Copy link
Member

Choose a reason for hiding this comment

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

Semantically this is equivalent with

dtype = dt.highest_precedence([self.arg.type(), dt.double])

probably faster though

Copy link
Member

@kszucs kszucs Jul 16, 2018

Choose a reason for hiding this comment

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

Which reminds me, We might decorate dt.highest_precedence with functools.lru_cache

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@kszucs kszucs left a comment

Choose a reason for hiding this comment

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

LGTM

@cpcloud cpcloud closed this in a9e8b06 Jul 16, 2018
@cpcloud cpcloud deleted the fix-decimal-support branch July 16, 2018 16:30
cpcloud added a commit that referenced this pull request Jul 17, 2018
Closes #1534   Depends on #1541

Author: Phillip Cloud <cpcloud@gmail.com>

Closes #1535 from cpcloud/add-bq-numeric and squashes the following commits:

03e7706 [Phillip Cloud] Remove unused variable
db5318e [Phillip Cloud] Cleanup dispatch
97675ea [Phillip Cloud] Add support for NUMERIC in the BigQuery backend 2
7012efe [Phillip Cloud] Add support for NUMERIC in the BigQuery backend
990d6fd [Phillip Cloud] Add datamgr numeric_table creation
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 expressions Issues or PRs related to the expression API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants