PR comments from apache#34311

[icexelloss]

Some comments from apache#34311

[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 - How to contribute patches

[rtpsw]

@icexelloss, the proposed changes are mostly OK. There is a conflict to resolve. I also suspect some automated conflict-resolutions did not go well, because I see both ResetAggregates and ResetKernelStates. Do you want to resolve the conflict(s) or should I? As for renaming symbols, I'm fine with RowSegmenter but SegmentPiece is odd, as a segment already means a piece. How about RowSegment for consistency?

[icexelloss]

I'm fine with RowSegmenter but SegmentPiece is odd, as a segment already means a piece. How about RowSegment for consistency?

I found the this confusing when first reading the code so I renamed this. Segment and segment piece are different concept. In my mind a segment is a set of rows with the same key and can span multiple batches. A segment piece a slice of a batch that belongs to a segment. I don't have strong opinions about the sames but if you want those are two things and we should be explicit about it. If what I describe as "segment piece" is what you call "segment" we need another term to represent "a collection of continuous segment that has the same key"

[icexelloss]

@rtpsw I named "Segment Piece" back to "Segment" and called the "a collection of continuous segment that has the same key" tentatively "segment group". Also resolved the conflict.

[rtpsw]

A segment piece a slice of a batch that belongs to a segment.

I see. OK, then I guess the name is fine, just needs a docstring.

[rtpsw]

Looks like the question mark here is a typo?

[icexelloss]

Fixed.

[rtpsw]

It's a bit confusing to start with a description of a segment group when the docstring is for segment. I'd say here something like "A segment is a chunk of contiguous rows that have the same segment key. Multiple contiguous segments having a common segment key form a segment group.".

Note that there is a typo: "that has" should be "that have".

[icexelloss]

I think it doesn't really make sense to talk about segment without talking about segment group first - segment group is the top-level concept, segment is how we track batches within a segment group

[rtpsw]

Typo: "span" should be "spans".

[icexelloss]

Good catch fixed.