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
[BEAM-11585] Select.flattenedSchema doesn't flatten nested array fields #14354
Conversation
retest this please |
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.
Everything is looking great! Just one suggestion to add an extended test description
@Test | ||
@Category(NeedsRunner.class) | ||
public void testFlatSchemaWithArrayNestedField() { | ||
|
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.
Let's add a description for this test, I suppose, this way we can provide a much better understanding to interested people. E.g. describe nested schema and expected flattened version of this schema.
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.
Done
retest this please |
Run Java PreCommit |
thanks @Amar3tto @KhaninArtur - is this ready for review/merge? @TheNeuralBit could you review once it's ready? : ) |
Sure, @Amar3tto please let me know when you're ready for a review. |
@TheNeuralBit please review |
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.
Thanks this looks good at first glance. Could you add some test-cases for edge cases, e.g. I'm curious if we handle:
Row[] transactions {
String[] banks,
Double purchaseAmount
}
->
String[][] transactions_banks,
Double[] transactions_purchaseAmount
and:
Row[] transactions {
Row[] banks {
String name
String address
},
Double purchaseAmount
}
->
String[][] transactions_banks_name,
String[][] transactions_banks_address,
Double[] transactions_purchaseAmount
Done, handled both cases. |
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.
LGTM, thank you!
@Amar3tto consider adding this to CHANGES.md for 2.30.0 as well |
Fixed
Select.flattenedSchema()
for nested array fields.Test case:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username
).[BEAM-XXX] Fixes bug in ApproximateQuantiles
, where you replaceBEAM-XXX
with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.