-
Notifications
You must be signed in to change notification settings - Fork 3k
Flink: Project the RowData from DeleteFilter to remove meta-columns. #2731
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
Conversation
| } | ||
|
|
||
| @Test | ||
| @Ignore("Disable it because the StructProjection will handle null value as non-null StructLike but throw NPE when" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed an issue for this comment: #2738
| } | ||
|
|
||
| public RowData project(RowData row) { | ||
| GenericRowData projectedRow = new GenericRowData(getters.length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RowDataIterator reuses object. this change deviates from that model and creates a copy for each row
| Iterator<RowData> rowDataIter = rowDataList.iterator(); | ||
|
|
||
| for (int i = 0; i < numRecords; i++) { | ||
| Assert.assertTrue("Should have more records", recordIter.hasNext()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it is necessary to have the assertions on line 149, 150, 158, 159. this is like to test the RandomGenericData.generate function.
even if we want to do, we can keep recordList and rowDataList are as List. Then we can just verify the size of the list
| } | ||
|
|
||
| @Test | ||
| public void testNestedProjection() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we probably need another test to verify the case that the top-level optional struct is null. randomly generated data won't cover that case
|
Since iceberg v2 expose by 0.12, more and more user encounter this problem. I think we should increase the priority of this RP and let it merge into master as soon as possible. If you don't have time to deal with this PR, I think I could pick this up and keep it in progress. :) @openinx |
|
@Reo-LEI , Pls go ahead. I can be the candidate reviewer for the PR, thanks. |
|
@openinx @stevenzwu I pick this up on #3240. Could you take a look of that? :) |
|
Closed via #3240. |
Address this issue : #2730