PartitionManager only allows `int32_t`-valued IDs — necessary restriction?

[alexandergunnarson]

Hey! First of all, amazing work on Quake!

I was wondering whether the below restriction in the PartitionManager is necessary:

// check ids are below max id
if ((vector_ids > std::numeric_limits<int32_t>::max()).any().item<bool>()) {
    throw runtime_error("[PartitionManager] add: vector_ids must be less than INT_MAX.");
}

If I were to comment this out, would problems happen? It appears int64_t IDs are used everywhere. Maybe some implementation detail relies on them being limited to the int32_t space?

Also, I wonder what the effect of using negative IDs is. They are technically permitted (except for -1, which is a sentinel value) — technically the minimum value of int64_t is allowed, so it seems an arbitrary cutoff to have such a large negative space available while the positive space gets cut off at int32_t.

Thanks!

[JasonMoho]

It is not a necessary restriction. Looks like a typo. We should check for max int64_t instead of int32_t.

And yes, we reserve -1 as a sentinel value, but in the PartitionManager::add, we should make sure that only positive partition IDs are added. We don't want to accidentally add negative/sentinel IDs.

Good catch, thanks!

[alexandergunnarson]

Okay awesome — thanks for the quick response!

In our case, 1) we've removed the restriction and 2) we use uint64_t for IDs and just cast them to int64_t, ensuring that a -1 ID (uint64_t max value) is disallowed. So maybe the check could be just id != MISSING_ID (where MISSING_ID = -1) as opposed to id >= 0/id > 0.