Immutable array based on RRB Vector

[jialunzhang-psu]

This PR modifies the tree structure based on RRB Vector: A Practical General Purpose Immutable Sequence, ICFP'15. It will support operations including removal and concat.

[coveralls]

Pull Request Test Coverage Report for Build 4819

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 200 of 229 (87.34%) changed or added relevant lines in 4 files are covered.
• 1 unchanged line in 1 file lost coverage.
• Overall coverage increased (+0.02%) to 83.15%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
immut/array/utils.mbt	16	17	94.12%
immut/array/array.mbt	27	30	90.0%
immut/array/tree_utils.mbt	19	30	63.33%
immut/array/tree.mbt	138	152	90.79%

Files with Coverage Reduction	New Missed Lines	%
ref/ref.mbt	1	71.43%

Totals
Change from base Build 4797:	0.02%
Covered Lines:	4999
Relevant Lines:	6012

---

💛 - Coveralls

[peter-jerry-ye-code-review]

‼️ This code review is generated by a bot. Please verify the content before trusting it.

After analyzing the git diff, here are three potential issues I've identified:

1. Memory Efficiency Issue in iter() method:

pub fn iter[A](self : T[A]) -> Iter[A] {
  Iter::new(fn(yield_) {
    let arr = self.to_array() // TODO: it first converts to an array, which is not efficient
    // ...
  })
}

There's a comment indicating that the current implementation first converts the entire structure to an array before iteration, which is inefficient in terms of memory usage. This could be optimized by implementing a direct iterator over the tree structure.

2. Potential Integer Overflow in redis_plan:

let total_nodes = node_counts.fold(init=0, fn { acc, x => acc + x })
// round up to the nearest integer of S/branching_factor
let opt_len = (total_nodes + branching_factor - 1) / branching_factor

There's no check for integer overflow when calculating total_nodes or opt_len. For very large arrays, this could lead to undefined behavior.

3. Potential Safety Issue in redis:

guard j < old_len  // This shouldn't be triggered if the plan was correctly generated

There's a comment suggesting that this guard should never fail if the plan is correct, but there's no validation of the plan before execution. If there's a bug in plan generation, this could lead to runtime errors. Consider adding validation of the redistribution plan before executing it.

These issues primarily affect performance, memory efficiency, and robustness of the implementation. Would you like me to elaborate on any of these points?

[jialunzhang-psu]

Originally I wanted to add concat and split and all other derived operations for immut/array, but now it turns out to be a large enough PR for just adding concat. So I pause here. There is more work to be done on this component, as listed in README.

In a nutshell, the main change of this PR is the introduction of the concat function in tree.mbt. All other things added are either helper functions or tests.

[bobzhang]

@jialunzhang-psu can you do a rebase? @Lampese are you interested in doing a review?

[Lampese]

@jialunzhang-psu can you do a rebase? @Lampese are you interested in doing a review?

OK, I will do it later.

[Lampese]

I'm not sure about the necessity of this API.
cc @peter-jerry-ye

[peter-jerry-ye]

Hmm, I don't think it's necessary. Even it were, maybe it can just return itself?

[jialunzhang-psu]

The API was already there when I touched it. I have no problem in removing it or making it private.

[Lampese]

IMO we need to remove it.

[jialunzhang-psu]

I removed it.