Make it possible to choose the index type (fixed selection)

[jeltsch]

This pull requests enables choice of index types via table configurations. It solves the same problem as #502 and #506 but using an alternative implementation, one that supports only a fixed selection of index types.

[jorisdral]

Almost LGTM! Some small suggestions and then after that we can merge

There are also some merge conflicts that have to be resolved, and we should probably squash the commits or rewrite the history a bit

[jorisdral]

Would it not be simpler to have:

{-|
    Adds information about appended pages to an index and outputs newly
    available chunks.

    See the documentation of the 'IndexAcc' type for constraints to adhere to.
-}
append :: Append -> IndexAcc s -> ST s [Chunk]
append instruction indexAcc = case instruction of
    AppendSinglePage minKey maxKey
        -> toList <$> Index.appendSingle (minKey, maxKey) indexAcc
    AppendMultiPage key overflowPageCount
        -> Index.appendMulti (key, overflowPageCount) indexAcc
{-# INLINE append #-}

{-|
    Adds information about appended pages to a compact index and outputs newly
    available chunks.

    See the documentation of the 'IndexAcc' type for constraints to adhere to.
-}
appendToCompact :: Append -> IndexCompactAcc s -> ST s [Chunk]
appendToCompact instruction indexAcc =
    append instruction (CompactIndexAcc indexAcc)
{-# INLINE appendToCompact #-}

{-|
    Adds information about appended pages to an ordinary index and outputs newly
    available chunks.

    See the documentation of the 'IndexAcc' type for constraints to adhere to.
-}
appendToOrdinary :: Append -> IndexOrdinaryAcc s -> ST s [Chunk]
appendToOrdinary instruction indexAcc =
    append instruction (OrdinaryIndexAcc indexAcc)
{-# INLINE appendToOrdinary #-}

[jorisdral]

Or even:

class AppendIndex j where
  {- |
       Adds information about appended pages to an index and outputs newly
       available chunks.

       See the documentation of the 'IndexAcc' type for constraints to adhere to.
  -}
  append :: Append -> j s -> ST s [Chunk]

instance AppendIndex IndexAcc where
  append instruction indexAcc = case instruction of
      AppendSinglePage minKey maxKey
        -> toList <$> Index.appendSingle (minKey, maxKey) indexAcc
      AppendMultiPage key overflowPageCount
        -> Index.appendMulti (key, overflowPageCount) indexAcc
  {-# INLINE append #-}

instance AppendIndex IndexCompactAcc where
  append instruction indexAcc =
      append instruction (CompactIndexAcc indexAcc)
  {-# INLINE append #-}

instance AppendIndex IndexOrdinaryAcc where
  append instruction indexAcc =
      append instruction (OrdinaryIndexAcc indexAcc)
  {-# INLINE append #-}

[jeltsch]

The first solution would be simpler but would also involve the indirection of going through the Index type even when the concrete index accumulator type is known. I considered something like the second solution before but in the end didn’t want to introduce a type class again just for this little bit of auxiliary code. All in all, I have a bias towards leaving the code in its current state. What do you think?

[jorisdral]

I don't think it would be bad to have an additional indirection for option 1, or an extra class for option 2. I'm fine with keeping it as is

[jeltsch]

There are also some merge conflicts that have to be resolved, and we should probably squash the commits or rewrite the history a bit.

I plan to squash the commits, including the ones inherited from #506, after the reviewing is complete, in order to not make reviewing harder. 🙂

[jeltsch]

And I plan to rebase the branch only after squashing, in order to make rebasing easier.

[jorisdral]

Suggested change

	appendWith :: ((SerialisedKey, SerialisedKey) -> j s -> ST s (Maybe Chunk))
	-> ((SerialisedKey, Word32) -> j s -> ST s [Chunk])
	-> Append
	-> j s
	-> ST s [Chunk]
	appendWith ::
	((SerialisedKey, SerialisedKey) -> j s -> ST s (Maybe Chunk))
	-> ((SerialisedKey, Word32) -> j s -> ST s [Chunk])
	-> Append
	-> j s
	-> ST s [Chunk]

[jorisdral]

I don't think it would be bad to have an additional indirection for option 1, or an extra class for option 2. I'm fine with keeping it as is