-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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-12442] Make sure that row renames work properly in nested rows. #14960
Conversation
R: @mouyang |
Run Java PreCommit |
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 think I'd prefer if we could just test this through the public interface rather than exposing internals, but this LG overall
Schema expectedSchema = | ||
Schema.builder() | ||
.addStringField("top1") | ||
.addRowField("top_nested", expectedNestedSchema) |
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.
optional super nitty nit: I think this would be easier to grok if the nested schemas were defined inline.
} | ||
})) | ||
.setRowSchema(outputSchema); | ||
} | ||
} | ||
|
||
// TODO(reuvenlax): For better performance, we should reuse functionality in |
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.
Consider a jira for this
default: | ||
return inputType; | ||
} | ||
} | ||
|
||
// Apply the user-specified renames to the input schema. | ||
private static Schema renameSchema(Schema inputSchema, Collection<RenamePair> renames) { | ||
@VisibleForTesting |
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.
Why not just test through the public interface, rather than exposing renameSchema
and renameRows
?
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'm not sure how. The problem with the public interface is that it uses the DirectRunner, and the characteristic of this bug is that the DirectRunner tends to obscure the bug by serializing/deserializing.
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.
Ah yeah. Shoot.
@reuvenlax LGTM. I have two general knowledge questions.
|
@mouyang correct on both counts |
No description provided.