You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@paimon.apache.org by "leaves12138 (via GitHub)" <gi...@apache.org> on 2025/01/24 06:28:05 UTC
[PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
leaves12138 opened a new pull request, #4992:
URL: https://github.com/apache/paimon/pull/4992
<!-- Please specify the module before the PR name: [core] ... or [flink] ... -->
### Purpose
<!-- Linking this pull request to the issue -->
Using LongIterator instead of long[].
<!-- What is the purpose of the change -->
### Tests
<!-- List UT and IT cases to verify this change -->
### API and Format
<!-- Does this change affect API or storage format -->
### Documentation
<!-- Does this change introduce a new feature -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4992:
URL: https://github.com/apache/paimon/pull/4992#discussion_r1928155457
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRowIterator.java:
##########
@@ -77,10 +78,15 @@ public InternalRow next() {
@Override
public long returnedPosition() {
- if (nextPos == 0) {
- return positions[0] - 1;
+ for (int i = 0; i < nextPos - callReturnedPositionTimes; i++) {
Review Comment:
Please add tests for this.
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRowIterator.java:
##########
@@ -52,16 +54,15 @@ public ColumnarRowIterator(Path filePath, ColumnarRow row, @Nullable Runnable re
}
public void reset(long nextFilePos) {
- reset(LongIterator.fromRange(nextFilePos, nextFilePos + positions.length));
+ reset(LongIterator.fromRange(nextFilePos, nextFilePos + row.batch().getNumRows()));
Review Comment:
You can rebase the master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4992:
URL: https://github.com/apache/paimon/pull/4992#discussion_r1928155326
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRowIterator.java:
##########
@@ -89,9 +95,11 @@ public Path filePath() {
}
protected ColumnarRowIterator copy(ColumnVector[] vectors) {
+ // We should call copy only when the iterator is at the beginning of the file.
+ assert nextPos == 0;
Review Comment:
Use `checkPrecondition`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4992:
URL: https://github.com/apache/paimon/pull/4992#discussion_r1928165529
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRowIterator.java:
##########
@@ -42,7 +42,9 @@ public class ColumnarRowIterator extends RecyclableIterator<InternalRow>
protected int num;
protected int nextPos;
Review Comment:
It is OK to rename this to index.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on PR #4992:
URL: https://github.com/apache/paimon/pull/4992#issuecomment-2611873625
Looks good to me!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4992:
URL: https://github.com/apache/paimon/pull/4992#discussion_r1928157210
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/ColumnarRowIterator.java:
##########
@@ -52,16 +54,15 @@ public ColumnarRowIterator(Path filePath, ColumnarRow row, @Nullable Runnable re
}
public void reset(long nextFilePos) {
- reset(LongIterator.fromRange(nextFilePos, nextFilePos + positions.length));
+ reset(LongIterator.fromRange(nextFilePos, nextFilePos + row.batch().getNumRows()));
}
public void reset(LongIterator positions) {
+ this.positionIterator = positions;
this.num = row.batch().getNumRows();
- this.positions = new long[num];
- for (int i = 0; i < num; i++) {
- this.positions[i] = positions.next();
- }
this.nextPos = 0;
+ this.callReturnedPositionTimes = 0;
Review Comment:
`returnedPositionIndex`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi commented on code in PR #4992:
URL: https://github.com/apache/paimon/pull/4992#discussion_r1928155698
##########
paimon-common/src/main/java/org/apache/paimon/data/columnar/VectorizedRowIterator.java:
##########
@@ -38,9 +37,10 @@ public VectorizedColumnBatch batch() {
@Override
protected VectorizedRowIterator copy(ColumnVector[] vectors) {
+ assert nextPos == 0;
Review Comment:
Use checkPrecondition
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [core] Refactory ColumnarRowIterator using LongIterator. [paimon]
Posted by "JingsongLi (via GitHub)" <gi...@apache.org>.
JingsongLi merged PR #4992:
URL: https://github.com/apache/paimon/pull/4992
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@paimon.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org