feat(7181): SortOrderBuilder and create composible Merge node #1

wiedld · 2023-10-24T20:56:03Z

~~This is branched off a~~ This builds off commits from a previous PR, which adds the CursorValues::slice().

This PR was rebased on latest main, in order to do performance testing. As such, the commits include the previous PR too (since that approved PR was not rebased).

Goal of this draft PR

Justify if we'll merge the previous PR (which adds CursorValues::slice), by demonstrating that the next steps (follow up PRs) will result in performance improvements.

Performance:

All branches use the same comparison main commit.
All done in isolation on gcp c3d-standard-8-lssd debian-11.

main vs removal-of-retain (a part of what this current draft PR does).
- Reduction of asymptotic complexity, produced significant performance improvements.
- Note: this code is an exact isolation of the code logic used in step (2) below -- which is this draft PR. In order to show where the improvements come from.

.

main vs single-composable-node (this draft PR).
- Has the same reduction of asymptotic complexity (as above).
- Does add more stream polling (slight performance impact).
- Does not yet have any multicore (that is a followup PR).

.

main vs cascaded-merge-tree (multicore)
- Has the same changes as both above.
- Also adds the ability to parallelize merge node sorting.

TODO -- (Re-doing with all iterative performance improvements; is not the same as the previous benchmarked cascaded tree. Almost ready to re-run benchmark here.)

… implementations

…ests to add CursorValue implementations

…and the BatchTrackerStream which converts

… and remove extra row_idx tracking in old BatchCursor

…ove closer to replacement BatchCursor

…n SortPreservingCascadeStream, by also moving the BatchBuilder to only yield sort_orders

…doc-links, and remove unused properties

…ore functionality than a cursor

…responsible

…arge datasets

…aTypes) (apache#8985) * ScalarValue return types from argument values * change file name * try using ?Sized * use Ok * move method default impl outside trait * Use type trait for ExprSchemable * fix nit * Proposed Return Type from Expr suggestions (#1) * Improve return_type_from_args * Rework example * Update datafusion/core/tests/user_defined/user_defined_scalar_functions.rs --------- Co-authored-by: Junhao Liu <[email protected]> * Apply suggestions from code review Co-authored-by: Alex Huang <[email protected]> * Fix tests + clippy * rework types to use dyn trait * fmt * docs * Apply suggestions from code review Co-authored-by: Jeffrey Vo <[email protected]> * Add docs explaining what happens when both `return_type` and `return_type_from_exprs` are called * clippy * fix doc -- comedy of errors --------- Co-authored-by: Andrew Lamb <[email protected]> Co-authored-by: Alex Huang <[email protected]> Co-authored-by: Jeffrey Vo <[email protected]>

* refactor `TreeNode::rewrite()` * use handle_tree_recursion in `Expr` * use macro for transform recursions * fix api * minor fixes * fix * don't trust `t.transformed` coming from transformation closures, keep the old way of detecting if changes were made * rephrase todo comment, always propagate up `t.transformed` from the transformation closure, fix projection pushdown closure * Fix `TreeNodeRecursion` docs * extend Skip (Prune) functionality to Jump as it is defined in https://synnada.notion.site/synnada/TreeNode-Design-Proposal-bceac27d18504a2085145550e267c4c1 * fix Jump and add tests * jump test fixes * fix clippy * unify "transform" traversals using macros, fix "visit" traversal jumps, add visit jump tests, ensure consistent naming `f` instead of `op`, `f_down` instead of `pre_visit` and `f_up` instead of `post_visit` * fix macro rewrite * minor fixes * minor fix * refactor tests * add transform tests * add apply, transform_down and transform_up tests * refactor tests * test jump on both a and e nodes in both top-down and bottom-up traversals * better transform/rewrite tests * minor fix * simplify tests * add stop tests, reorganize tests * fix previous merges and remove leftover file * Review TreeNode Refactor (#1) * Minor changes * Jump doesn't ignore f_up * update test * Update rewriter * LogicalPlan visit update and propagate from children flags * Update tree_node.rs * Update map_children's --------- Co-authored-by: Mustafa Akur <[email protected]> * fix * minor fixes * fix f_up call when f_down returns jump * simplify code * minor fix * revert unnecessary changes * fix `DynTreeNode` and `ConcreteTreeNode` `transformed` and `tnr` propagation * introduce TransformedResult helper * fix docs * restore transform as alias to trassform_up * restore transform as alias to trassform_up 2 * Simplifications and comment improvements (#2) --------- Co-authored-by: Berkay Şahin <[email protected]> Co-authored-by: Mustafa Akur <[email protected]> Co-authored-by: Mehmet Ozan Kabak <[email protected]>

github-actions · 2024-08-08T02:24:51Z

Thank you for your contribution. Unfortunately, this pull request is stale because it has been open 60 days with no activity. Please remove the stale label or comment or this will be closed in 7 days.

wiedld added 15 commits October 23, 2023 19:12

feat(7181): provide slicing of CursorValues, and test for the various…

6cd1b82

… implementations

fix(7181): memory reservation is required, but pass by rc'ed

9510072

fix(7181): upper bounds check should be non-inclusive for RowValues

6156a57

fix(7181): fix bounds check on RowValues::slice, and add regression t…

af1a554

…ests to add CursorValue implementations

feat(7181): add batches mod, with BatchCursor and BatchTracker

656f9fa

feat(7181): add type definitions of BatchStream & BatchCursorStream, …

0205171

…and the BatchTrackerStream which converts

feat(7181): move cursors into BatchBuilder (future SortOrderBuilder),…

527bdc6

… and remove extra row_idx tracking in old BatchCursor

feat(7181): remove option from old BatchCursor.cursor, such that we m…

f40a8b4

…ove closer to replacement BatchCursor

feat(7181): build I/O streams around a single composable merge node i…

abe7ab1

…n SortPreservingCascadeStream, by also moving the BatchBuilder to only yield sort_orders

feat(7181): handle the abs_idx difference in sliced CursorValues

cc998d2

refactor(7181): rename BatchBuilder to SortOrderBuilder, cleanup all …

138c697

…doc-links, and remove unused properties

chore(7181): rename BatchCursor to BatchRowSet, as it contains alot m…

66383f6

…ore functionality than a cursor

chore(7181): move the record_batch yield diagram, over to the method …

f9d7be3

…responsible

chore(7181): add stream diagram for composable merge node

a5b60a3

fix(7181): debug CursorValues slicing and handling, over tests with l…

1ff4fa1

…arge datasets

This was referenced Oct 24, 2023

feat(7181): Encapsulate cursor handling within the SortOrderBuilder. apache/datafusion#7842

Closed

feat(7181): provide slicing of CursorValues apache/datafusion#7912

Closed

Merge branch 'main' into 7181/sort-order-and-composible-node

e74ba5d

wiedld changed the base branch from 7181/slice-cursor-values to main October 28, 2023 17:49

refactor(7181): use hashmap entry for mutable ref

a3bd193

wiedld force-pushed the 7181/sort-order-and-composible-node branch from b123748 to a3bd193 Compare October 29, 2023 02:24

github-actions bot added the Stale label Aug 8, 2024

github-actions bot closed this Aug 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(7181): SortOrderBuilder and create composible Merge node #1

feat(7181): SortOrderBuilder and create composible Merge node #1

wiedld commented Oct 24, 2023 •

edited

Loading

github-actions bot commented Aug 8, 2024

feat(7181): SortOrderBuilder and create composible Merge node #1

feat(7181): SortOrderBuilder and create composible Merge node #1

Conversation

wiedld commented Oct 24, 2023 • edited Loading

Goal of this draft PR

Performance:

github-actions bot commented Aug 8, 2024

wiedld commented Oct 24, 2023 •

edited

Loading