-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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-17812: [Gandiva][Docs] Add C++ Gandiva User Guide #14200
ARROW-17812: [Gandiva][Docs] Add C++ Gandiva User Guide #14200
Conversation
FYI @js8544 |
Thanks! |
@pitrou Would you mind reviewing this PR? I am planning on improving Gandiva's doc based on Will's work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice and well-written, thanks a lot @wjones127 !
docs/source/cpp/gandiva.rst
Outdated
Gandiva was designed to take advantage of the Arrow memory format and modern | ||
hardware. Compiling expressions using LLVM allows the execution to be optimized | ||
to the local runtime environment and hardware, including available SIMD | ||
instructions. To minimize optimization overhead, all Gandiva functions are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really all Gandiva functions (including e.g. simple additions) or the most costly ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I would say "To reduce optimization overhead".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed to "many", though I'm not sure which is right. I wrote this about a year ago, based on what I could understand from the Arrow and Dremio blog posts. But I've forgotten where I might have gotten this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pitrou As far as I understand all functions are precompiled to LLVM IR. For example addition is defined here as a C function: https://github.com/apache/arrow/blob/master/cpp/src/gandiva/precompiled/arithmetic_ops.cc#L85. During the build process, all functions will first be compiled to LLVM IR format(https://github.com/apache/arrow/blob/master/cpp/src/gandiva/precompiled/CMakeLists.txt#L88) and loaded by the LLVM engine before any computation occurs (https://github.com/apache/arrow/blob/master/cpp/src/gandiva/engine.cc#L257).
@ksuarez1423 Do you want to take a look at the additions here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this is really nice! I really like the flow of explaining trees, then evaluation of trees, without overlap. That really makes things easier to follow, and I left a comment at each place that I was a little lost.
==================== | ||
|
||
Gandiva provides a general expression representation where expressions are | ||
represented by a tree of nodes. The expression trees are built using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth explaining what the nodes represent in particular. While I can figure it out in the next section, where function nodes are discussed, having to wait for that clarity feels weird to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what would really be helpful would be to draw an expression tree, but idk if I want to do that in this PR.
I don't really know how to describe what a node itself means. That's why I first describe the leaves, and then the composite nodes. And then give an example.
If you have a suggestion, though, I'd happily take it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something's off in the writing, then -- I did not find myself thinking of "composite nodes" after reading, only leaf nodes, then something that's above that. It might be worth going more in depth in an additional paragraph, at least.
As for how to describe them, internal nodes represent operations that handle multiple leaves, while leaves contain fields and literals, I believe? I haven't written any Gandiva, so if I'm off-key here, we could talk further.
|
||
Once a Projector or Filter is created, it can be evaluated on Arrow record batches. | ||
These execution kernels are single-threaded on their own, but are designed to be | ||
reused to process distinct record batches in parallel. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could an example be provided of how to do this? Even if it's just "use them in your OpenMP threads," that still provides some useful guidance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH I'm not familiar enough with multi-threading in C++ to have a ready example for that. We have some task and thread pool machinery in Arrow, but that's not what users would be expected to use.
@github-actions crossbow submit preview-docs |
Revision: 79deadc Submitted crossbow builds: ursacomputing/crossbow @ actions-3d38fbfa71
|
Co-authored-by: Antoine Pitrou <antoine@python.org>
3099fe0
to
7149a2c
Compare
@github-actions crossbow submit preview-docs |
Revision: 7149a2c Submitted crossbow builds: ursacomputing/crossbow @ actions-2243107e33
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Benchmark runs are scheduled for baseline = 8a5aa67 and contender = 5182d62. 5182d62 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
['Python', 'R'] benchmarks have high level of regressions. |
No description provided.