Enable AST-based joining #8214
Enable AST-based joining #8214
Conversation
vyasr
added
feature request
Codecov Report
@@ Coverage Diff @@
## branch-21.08 #8214 +/- ##
===============================================
Coverage ? 10.67%
===============================================
Files ? 109
Lines ? 18667
Branches ? 0
===============================================
Hits ? 1993
Misses ? 16674
Partials ? 0 Continue to review full report at Codecov.
|
vyasr
force-pushed
the
feature/ast_equijoin
branch
from
25473b3 to
692f801
Compare
github-actions
bot
added
CMake
vyasr
force-pushed
the
feature/ast_equijoin
branch
from
692f801 to
c77a857
Compare
vyasr
force-pushed
the
feature/ast_equijoin
branch
from
2d87e06 to
bde476e
Compare
Notes on performanceExisting APIsThe changes to the AST evaluation have a largely negligible effect on the performance of preexisting code paths. For reference, here are the benchmarks of the And here are the benchmarks on this branch There is a small but reproducible additional fixed cost coming from some of these changes (mostly from some of the abstractions I introduced like the New join codeWith respect to the join code, using the benchmarks posted on #5397 (the benchmarks vs hash join) as a baseline we see roughly a 4x slowdown associated with joining using the AST as compared to using a raw equality comparison. Here are the numbers copy-pasted from that PR: And here are the numbers now. I haven't done any rigorous analysis yet, but offhand this performance hit doesn't seem terribly surprising. There are a number of potential contributing factors:
All told, a 4x performance hit for those reasons doesn't seem unreasonable. |
|
@jrhemstad the issue I raised at the meeting today is in the
If we choose to leave the code as is, we could use something like CRTP to more clearly delineate an API, but that seems pretty heavy-handed for something internal-facing like this. EDIT |
|
A couple additional notes on outstanding tasks.
EDIT 6/18 |
There was a problem hiding this comment.
@vyasr Thanks for pinging me, it's great to see what you did with this PR. This looks like a good architecture, overall. I caught one bug where left should be right. I put in some minor comments on docstrings and constness. I probably will not have time to read this again in the next 2-3 weeks, so I'm submitting this PR review as a comment rather than requesting changes/approving.
…quijoin # Conflicts: # cpp/src/join/hash_join.cu
| table_view left, | ||
| table_view right, | ||
| ast::expression binary_predicate, | ||
| null_equality compare_nulls = null_equality::EQUAL, |
There was a problem hiding this comment.
Since the caller can specify the join condition via the AST, I'm surprised to see this parameter here. Instead I would expect there to be either two types of AST equality expressions or a parameterized AST equality operator to control the null behavior. The caller would then build the appropriate type of AST to match the null handling they need.
There was a problem hiding this comment.
Yeah, we were just talking about this earlier today. It quickly devolved into asking whether or not we need two versions of every operator that has different null behavior. We kept the null_equality for now to be consistent with the other join APIs, but I agree it is weird to have it and we should do something better.
There was a problem hiding this comment.
@jrhemstad and I had a similar discussion earlier today, and I think we decided moving in that direction makes sense as well.
There was a problem hiding this comment.
I think the only operators for which we might want this are = and !=, right? I can't imagine reasonable semantics for any other operator. != is borderline, but I can imagine that other tools might try to define different semantics for NULL != NULL, I just don't know what those are for consumers of this API.
There was a problem hiding this comment.
I suspect it will only be necessary for ==. That being said, in the future it could be very useful in an AST expression to conditionally branch based on the result of an "is this null" operator. Then users could implement their own fancy-pants versions of null comparison or other special-case null handling in their custom expressions.
|
@gpucibot merge |
|
@jrhemstad @codereport I did some more in-depth profiling against the old nested loop join and got back some useful information, as well as some good news. As expected, the biggest differences are:
Here's a PDF of the nsight-compute comparison profile if you'd like to see it. The good news is that in benchmarking the nested loop join code I realized I was making the incorrect assumption that I was using comparable hardware to what was used for the benchmarks on #5397. It turns out the the benchmarks I posted above on this PR were actually on a slower GPU (Tesla T4) than I suspect was used for the benchmarks on #5397 (something comparable to an RTX 8000, which I now have benchmarks on). For comparison, here are the results on a Tesla T4: and here are the results on an RTX 8000 As you can see, the original nested join loop benchmark is nearly identical to the RTX 8000 one, whereas my conditional join benchmarks have been run on the T4. Evidently the slowdown is ~2x for the 100k * 100k join, and actually meaningfully less than 2x for the larger 100k * 1MM join, so we are in better shape than I had originally reported. |
… outstanding PR comments from PR rapidsai#8214.
… outstanding PR comments from PR rapidsai#8214.
#8214 made use of a number of functions defined for hash joins, but to expedite the review process no attempt was made to improve the organization of code into shared files. This PR improves that organization for improved conceptual clarity and faster parallel compilation and recompilation times. As a result, most of the changes in this PR are simply preexisting code to new files, as well as removing a number of unnecessary headers. The main new code is a few minor improvements that were suggested but not implemented on #8214 are included, mainly 1) a new `nullable(table_view const&)` API for checking if any columns in a table are nullable (requested [here](#8214 (comment))), and 2) some extra comments on the behavior of conditional joins when the condition returns null (requested [here](#8214 (comment))). Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Mark Harris (https://github.com/harrism) - Mike Wilson (https://github.com/hyperbolic2346) URL: #8815
This PR implements conditional joins using expressions that are decomposed into abstract syntax trees for evaluation. This PR builds on the AST evaluation framework established in #5494 and #7418, but significantly refactors the internals and generalizes them to enable 1) expressions on two tables and 2) operations on nullable columns. This PR uses the nested loop join code created in #5397 for inner joins, but also substantially generalizes that code to enable 1) all types of joins, 2) joins with arbitrary AST expressions rather than just equality, and 3) handling of null values (with user-specified
null_equality). A significant chunk of the code is currently out of place, but since this changeset is rather large I've opted not to move things in ways that will make reviewing this PR significantly more challenging. I will make a follow-up to address those issues once this PR is merged.