GH-40020: [C++] Change offset types to signed in row table related structures and APIs

[zanmato1984]

Rationale for this change

As described in #40020.

What changes are included in this PR?

Change all "offset" to signed types in row table.
And all the transitive changes to other data structures and APIs - I know, there are a lot!
Removed several static cast.

Are these changes tested?

The existing tests should cover them good.

Are there any user-facing changes?

No.

• Closes: [C++] Use signed offset type in row table related structures #40020

[github-actions]

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[zanmato1984]

cc @pitrou

[westonpace]

Thanks for your interest!

What's the motivation for this change? Static casts like this should be pretty much free. I worry that some of these values do in fact need to be unsigned (I'm pretty sure we are limited to 4GiB of accumulation in the hash join and I think this might drop that to 2GiB and/or introduce bugs).

[zanmato1984]

Thanks for your interest!

What's the motivation for this change? Static casts like this should be pretty much free. I worry that some of these values do in fact need to be unsigned (I'm pretty sure we are limited to 4GiB of accumulation in the hash join and I think this might drop that to 2GiB and/or introduce bugs).

Thanks for looking. I don't actually have a strong motivation except making the code more conventional to the rest of the project. Didn't know there is consideration about supporting hash join bigger than 2GB. If this is the case then I think we can close this.

[pitrou]

So, there are two things:

• all offsets of variable-width types (e.g. Binary, List...) are signed in Arrow; they should not be treated as unsigned, regardless of their widths
• if some other 32-bit fields might overflow because they denote a number of rows or a number of bytes, then they should be converted to 64-bit instead of pedantically insisting on unsigned 32-bit

[zanmato1984]

So, there are two things:

• all offsets of variable-width types (e.g. Binary, List...) are signed in Arrow; they should not be treated as unsigned, regardless of their widths

• if some other 32-bit fields might overflow because they denote a number of rows or a number of bytes, then they should be converted to 64-bit instead of pedantically insisting on unsigned 32-bit

+1

Maybe I can work out some edge test cases to identify what the blockers are if we have both signed offset types and over 2GB hash join ability.

[westonpace]

all offsets of variable-width types (e.g. Binary, List...) are signed in Arrow; they should not be treated as unsigned, regardless of their widths

I'm fairly certain none of these fall into that category. These are row-offsets and byte-offsets into the "row table" that the hash-join uses for intermediate storage of data.

if some other 32-bit fields might overflow because they denote a number of rows or a number of bytes, then they should be converted to 64-bit instead of pedantically insisting on unsigned 32-bit

64-bit here should be fine. This is the defect I was thinking of originally: #34474

As long as we are only changing uint32_t to int64_t and all the tests pass then I don't think we need to worry about breaking large memory support.

[zanmato1984]

64-bit here should be fine. This is the defect I was thinking of originally: #34474

As long as we are only changing uint32_t to int64_t and all the tests pass then I don't think we need to worry about breaking large memory support.

Hi @westonpace , sorry about coming back to this late. In the quoted comment, are you suggesting keeping the row-offsets/byte-offsets unsigned and only changing the number of rows/bytes to signed 64-bit? Or changing both the offsets and numbers to signed 64-bit? Please help to confirm and I'll keep working on this. Thank you.

[zanmato1984]

Since current row table code has been evolved and I've been looking at the code more, I might be able to come up with a more thorough improvement to row table in the future than merely changing the offset types. So I'm closing this PR for now.