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

ARROW-5626: [C++] Fix caching of expressions with decimals #4592

Closed
wants to merge 1 commit into from

Conversation

pravindra
Copy link
Contributor

  • use full DataType instead of just name
  • added test for caching decimal expr

- use full DataType instead of just name
- added test for caching decimal expr
Copy link
Contributor

@praveenbingo praveenbingo left a comment

Choose a reason for hiding this comment

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

+1 LGMT.

@fsaintjacques fsaintjacques changed the title ARROW-5626: fix caching of expressions with decimals ARROW-5626: [C++] Fix caching of expressions with decimals Jun 17, 2019
Copy link
Contributor

@fsaintjacques fsaintjacques left a comment

Choose a reason for hiding this comment

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

The fix/implementation depends on the Type::ToString method to return a unique identifier based on the (runtime) parameters. This is an implicit contract. I think it's worth adding a class that is simply a TypeVisitor that returns a key and properly warn when a type is unknown (and not cachable).

@codecov-io
Copy link

Codecov Report

Merging #4592 into master will decrease coverage by 13.25%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #4592       +/-   ##
===========================================
- Coverage   88.57%   75.31%   -13.26%     
===========================================
  Files         860       56      -804     
  Lines      108022     3192   -104830     
  Branches     1253        0     -1253     
===========================================
- Hits        95678     2404    -93274     
+ Misses      12065      788    -11277     
+ Partials      279        0      -279
Impacted Files Coverage Δ
python/pyarrow/ipc.pxi
cpp/src/arrow/csv/chunker-test.cc
cpp/src/parquet/column_page.h
cpp/src/parquet/bloom_filter-test.cc
cpp/src/arrow/array/builder_decimal.cc
cpp/src/plasma/client.cc
cpp/src/arrow/io/test-common.h
cpp/src/arrow/util/int-util-test.cc
cpp/src/arrow/python/io.cc
python/pyarrow/hdfs.py
... and 790 more

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 9425831...7506c00. Read the comment docs.

@pravindra
Copy link
Contributor Author

The fix/implementation depends on the Type::ToString method to return a unique identifier based on the (runtime) parameters. This is an implicit contract. I think it's worth adding a class that is simply a TypeVisitor that returns a key and properly warn when a type is unknown (and not cachable).

agree - we've already hit a couple of bugs related to this. I'll open a jira for this.

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.

None yet

4 participants