-
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
Cleans up PTransform validation across Beam #3730
Conversation
public void validate() { | ||
spec.validate(null); | ||
} | ||
public void validate() {} |
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.
rm
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 (had to introduce a default empty validate() method in Source, which is probably a good thing anyway, and I removed a bunch more empty validate() methods)
return builder().setMaxReadTime(maxReadTime).build(); | ||
} | ||
|
||
@Override | ||
public PCollection<byte[]> expand(PBegin input) { | ||
checkArgument(maxReadTime() == null, | ||
"withMaxNumRecords() and withMaxReadTime() are exclusive"); |
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.
This doesn't seem to check both
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
public void validate() { | ||
spec.validate(null); | ||
} | ||
public void validate() {} |
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.
rm
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
@@ -282,9 +270,7 @@ private BoundedMongoDbSource(Read spec) { | |||
} | |||
|
|||
@Override | |||
public void validate() { | |||
spec.validate(null); | |||
} |
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.
rm
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
checkArgument(numSplits >= 0, "MongoDbIO.read().withNumSplits(numSplits) called with " | ||
+ "invalid number. The number of splits has to be a positive value (currently %d)", | ||
numSplits); | ||
checkArgument(numSplits >= 0, "invalid num_splits: must be > 0, but was %d", numSplits); |
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.
This message and check don't agree on the requirement
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
checkNotNull(parseFn, "parseFn"); | ||
checkNotNull(parseFn, "coder"); | ||
checkArgument(parseFn != null, "parseFn can not be null"); | ||
checkArgument(parseFn != null, "parseFn can not be null"); |
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.
coder
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
table != null || getQuery() != null, | ||
"Invalid BigQueryIO.Read: one of table reference and query must be set"); | ||
|
||
if (table != null) { |
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 may be able to collapse these a bit;
if (table == null) {
checkArgument(query != null, ...)
// query validation
} else {
checkArgument(query == null, ...)
// table validation
}
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
@@ -540,6 +512,37 @@ public void validate(PipelineOptions options) { | |||
|
|||
@Override | |||
public PCollection<TableRow> expand(PBegin input) { | |||
ValueProvider<TableReference> table = getTableProvider(); | |||
|
|||
checkState( |
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.
checkArguments?
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
type != null, | ||
"ConnectionConfiguration.create(addresses, index, type) called with null type"); | ||
checkArgument(addresses != null, "addresses can not be null"); | ||
checkArgument(addresses.length != 0, "addresses can not be empty"); |
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.
length > 0, ...
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
public void validate() { | ||
spec.validate(null); | ||
} | ||
public void validate() {} |
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.
rm
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
Changes Unknown when pulling e9bbabd on jkff:validate-cleanup into ** on apache:master**. |
6ff7b24
to
45b267b
Compare
- Moves validation from validate() to expand() when possible - Replaces checkNotNull() with checkArgument() when possible - Shortens overly verbose validation messages in a few IOs
45b267b
to
7b81dcb
Compare
Rebased, PTAL. |
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.
LGTM
@@ -1130,6 +1122,12 @@ public DeleteKey withProjectId(ValueProvider<String> projectId) { | |||
|
|||
@Override | |||
public PDone expand(PCollection<T> input) { | |||
checkArgument(projectId != null, "withProjectId() is required"); | |||
if (projectId.isAccessible()) { |
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.
My meaning is that it fits the appropriate mental model of when validation occurs rather than it will accept more pipelines properly
As a piggyback, one of the sites to be cleaned up was AssignWindows which happened to be unused (it is also unused by Dataflow Worker), so I deleted it.
R: @tgroh