Skip to content

ARROW-4102: [C++] Return common IdentityCast when casting to equal type#3265

Closed
wesm wants to merge 2 commits intoapache:masterfrom
wesm:ARROW-4102
Closed

ARROW-4102: [C++] Return common IdentityCast when casting to equal type#3265
wesm wants to merge 2 commits intoapache:masterfrom
wesm:ARROW-4102

Conversation

@wesm
Copy link
Copy Markdown
Member

@wesm wesm commented Dec 26, 2018

I also added some code to make it easier to write cast tests in JSON.

As one issue with the JSON parser -- we have a number of tests in cast-test.cc that check that values that are in null positions are ignored. We might augment the parser to be able to pass both values and validity bitmap as separate JSON strings

wesm added 2 commits December 26, 2018 16:04
…ty casts work now

Change-Id: If96fb15fd2947cb69bc22ef3579628eaba48709b
Change-Id: Ic723453c9f7f0558c23aa7efe191c456965fc1e0
@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #3265 into master will increase coverage by 1.2%.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3265     +/-   ##
========================================
+ Coverage    88.5%   89.7%   +1.2%     
========================================
  Files         540     482     -58     
  Lines       72917   68928   -3989     
========================================
- Hits        64535   61834   -2701     
+ Misses       8279    7094   -1185     
+ Partials      103       0    -103
Impacted Files Coverage Δ
python/pyarrow/tests/test_array.py 97.28% <ø> (ø) ⬆️
cpp/src/arrow/compute/kernels/cast.cc 91.87% <100%> (-0.06%) ⬇️
cpp/src/arrow/compute/kernels/cast-test.cc 99.54% <100%> (ø) ⬆️
cpp/src/arrow/ipc/json-simple.cc 99.17% <100%> (+0.01%) ⬆️
go/arrow/array/table.go
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
... and 53 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 46ecbb6...8c27ba2. Read the comment docs.

Copy link
Copy Markdown
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.

just for my understanding..so we now have two kernels? one for gandiva and one within arrow compute?
what is the tenet for the later - are these common util functions that are performed on arrow data and might not be expressions?

@wesm
Copy link
Copy Markdown
Member Author

wesm commented Dec 27, 2018

@praveenbingo these have been here for quite a while -- since the arrival of the Gandiva codebase there has been no discussion yet of how to handle individual eager kernel invocation as well as fused / compiled expressions. I am also not sure we want to introduce LLVM as a requirement into all applications. To require LLVM to be able to cast from int8 to int64 seems like a lot to ask.

We should try to avoid duplicating pre-compiled kernel implementations between Gandiva/JIT-compilation and other parts of the project.

Just to prepare yourselves -- over the next couple of years, there's very likely to be significant expansion of function kernels and an in-memory / multi-core query engine with the objective of using them in data science applications. This will eventually include predicate pushdown, projections, filters, aggregations, joins, and various kinds of time series operations, among other things.

Copy link
Copy Markdown
Member

@xhochy xhochy left a comment

Choose a reason for hiding this comment

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

+1, LGTM

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.

4 participants