Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flink](lookup) project row to avoid deserialize unnecessary fields #148

Merged
merged 2 commits into from
Dec 26, 2024

Conversation

JNSimba
Copy link
Contributor

@JNSimba JNSimba commented Dec 9, 2024

Purpose

project row to avoid deserialize unnecessary fields

// to avoid deserialize unnecessary fields
RowData flinkRow = flussRowToFlinkRowConverter.toFlinkRowData(row);
RowData flinkRow =
flussRowToFlinkRowConverter.toFlinkRowData(
Copy link
Member

@wuchong wuchong Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flussRowToFlinkRowConverter works on full fields and it fails ArrayIndexOutOfBoundsException if passing projected row. Try run com.alibaba.fluss.connector.flink.source.FlinkTableSourceITCase#testLookup1PkTable.

flussRowToFlinkRowConverter should also be adapted for projection if we want it work on projected row.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, it has been modified, and FlinkTableSourceITCase#testLookup1PkTable has been successfully run

@wuchong
Copy link
Member

wuchong commented Dec 16, 2024

I rebased the branch onto main to trigger CI tests.

@JNSimba JNSimba requested a review from wuchong December 19, 2024 07:17
@JNSimba
Copy link
Contributor Author

JNSimba commented Dec 19, 2024

@wuchong comments addressed, PTAL, Thanks.

Copy link
Member

@wuchong wuchong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a commit to address the problems.

new FlussRowToFlinkRowConverter(FlinkConversions.toFlussRowType(flinkRowType));
new FlussRowToFlinkRowConverter(
FlinkConversions.toFlussRowType(
FlinkLookupFunction.filterRowType(flinkRowType, projection)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

projection may be null, this can cause NPE

for (int i = 0; i < pkIndex.length; i++) {
types[i] = rowType.getTypeAt(pkIndex[i]);
names[i] = rowType.getFieldNames().get(pkIndex[i]);
static RowType filterRowType(RowType rowType, int[] filterIndex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally, this is called projectRowType and we can move this to a common util.

RowData flinkRow = flussRowToFlinkRowConverter.toFlinkRowData(row);
RowData flinkRow =
flussRowToFlinkRowConverter.toFlinkRowData(
ProjectedRow.from(projection).replaceRow(row));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can cause NPE as the projection can be null.

@wuchong wuchong merged commit 2700ef5 into alibaba:main Dec 26, 2024
2 checks passed
wuchong pushed a commit that referenced this pull request Dec 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants