kennknowles commented on code in PR #17165:
URL: https://github.com/apache/beam/pull/17165#discussion_r953156869
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/coders/BigDecimalCoder.java:
##########
@@ -55,7 +55,8 @@ public void encode(BigDecimal value, OutputStream outStream)
throws IOException,
@Override
public void encode(BigDecimal value, OutputStream outStream, Context context)
throws IOException, CoderException {
- checkNotNull(value, String.format("cannot encode a null %s",
BigDecimal.class.getSimpleName()));
+ checkArgumentNotNull(
Review Comment:
Since this is actually a hot path, it would be good to avoid the string
formatting like this:
`checkArgumentNotNull(value, "cannot encode a null %s",
BigDecimal.class.getSimpleName())`
It seems that there are many opportunities for this.
I know this is already in the code. You could make them separate commits
since they are independent changes. Or you can skip that and someone else can
do it later.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java:
##########
@@ -922,7 +922,7 @@ public ReadAll<T> withBeamSchemas(boolean withBeamSchemas) {
@Override
public PCollection<T> expand(PCollection<String> input) {
- checkNotNull(getSchema(), "schema");
+ checkArgumentNotNull(getSchema(), "schema");
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java:
##########
@@ -1049,7 +1049,7 @@ public Parse<T> withHintMatchesManyFiles() {
@Override
public PCollection<T> expand(PBegin input) {
- checkNotNull(getFilepattern(), "filepattern");
+ checkArgumentNotNull(getFilepattern(), "filepattern");
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java:
##########
@@ -685,8 +685,8 @@ public Read<T> withBeamSchemas(boolean withBeamSchemas) {
@Override
@SuppressWarnings("unchecked")
public PCollection<T> expand(PBegin input) {
- checkNotNull(getFilepattern(), "filepattern");
- checkNotNull(getSchema(), "schema");
+ checkArgumentNotNull(getFilepattern(), "filepattern");
Review Comment:
These two should be `checkStateNotNull` because they are not referring to
arguments.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/AvroIO.java:
##########
@@ -825,7 +825,7 @@ public ReadFiles<T>
withDatumReaderFactory(AvroSource.DatumReaderFactory<T> fact
@Override
public PCollection<T> expand(PCollection<ReadableFile> input) {
- checkNotNull(getSchema(), "schema");
+ checkArgumentNotNull(getSchema(), "schema");
Review Comment:
Here also `checkStateNotNull`. And if you feel like making a better error
message, that is nice too.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/CompressedSource.java:
##########
@@ -244,9 +244,9 @@ private CompressedSource(
@Override
public void validate() {
super.validate();
- checkNotNull(sourceDelegate);
+ checkArgumentNotNull(sourceDelegate);
sourceDelegate.validate();
- checkNotNull(channelFactory);
+ checkArgumentNotNull(channelFactory);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/Read.java:
##########
@@ -525,8 +525,8 @@ public void splitRestriction(
restrictionTracker(
@Restriction UnboundedSourceRestriction<OutputT, CheckpointT>
restriction,
PipelineOptions pipelineOptions) {
- checkNotNull(restrictionCoder);
- checkNotNull(cachedReaders);
+ checkArgumentNotNull(restrictionCoder);
+ checkArgumentNotNull(cachedReaders);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/CompressedSource.java:
##########
@@ -244,9 +244,9 @@ private CompressedSource(
@Override
public void validate() {
super.validate();
- checkNotNull(sourceDelegate);
+ checkArgumentNotNull(sourceDelegate);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/WriteFiles.java:
##########
@@ -883,9 +883,10 @@ public void processElement(ProcessContext context) throws
Exception {
if (numShardsView != null) {
shardCount = context.sideInput(numShardsView);
} else {
- checkNotNull(getNumShardsProvider());
+ checkArgumentNotNull(getNumShardsProvider());
shardCount =
- checkNotNull(getNumShardsProvider().get(), "Must have non-null
number of shards.");
+ checkArgumentNotNull(
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/options/SdkHarnessOptions.java:
##########
@@ -168,7 +168,7 @@ class DefaultMaxCacheMemoryUsageMbFactory implements
DefaultValueFactory<@NonNeg
public @NonNegative Integer create(PipelineOptions options) {
SdkHarnessOptions sdkHarnessOptions =
options.as(SdkHarnessOptions.class);
return (Integer)
- checkNotNull(
+ checkArgumentNotNull(
Review Comment:
This one is strange. Probably `checkStateNotNull` since it indicates an
invalid state reached due to this code failing.
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/Read.java:
##########
@@ -817,7 +817,7 @@ public CheckpointMark getCheckpointMark() {
private Object createCacheKey(
UnboundedSource<OutputT, CheckpointT> source, CheckpointT
checkpoint) {
- checkNotNull(restrictionCoder);
+ checkArgumentNotNull(restrictionCoder);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/WriteFiles.java:
##########
@@ -883,9 +883,10 @@ public void processElement(ProcessContext context) throws
Exception {
if (numShardsView != null) {
shardCount = context.sideInput(numShardsView);
} else {
- checkNotNull(getNumShardsProvider());
+ checkArgumentNotNull(getNumShardsProvider());
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/values/Row.java:
##########
@@ -842,7 +842,7 @@ public Row withFieldValueGetters(
}
public Row build() {
- checkNotNull(schema);
+ checkArgumentNotNull(schema);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Watch.java:
##########
@@ -731,8 +731,8 @@ public Growth<InputT, OutputT, KeyT>
withOutputCoder(Coder<OutputT> outputCoder)
@Override
public PCollection<KV<InputT, OutputT>> expand(PCollection<InputT> input) {
- checkNotNull(getPollInterval(), "pollInterval");
- checkNotNull(getTerminationPerInput(), "terminationPerInput");
+ checkArgumentNotNull(getPollInterval(), "pollInterval");
+ checkArgumentNotNull(getTerminationPerInput(), "terminationPerInput");
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/Watch.java:
##########
@@ -731,8 +731,8 @@ public Growth<InputT, OutputT, KeyT>
withOutputCoder(Coder<OutputT> outputCoder)
@Override
public PCollection<KV<InputT, OutputT>> expand(PCollection<InputT> input) {
- checkNotNull(getPollInterval(), "pollInterval");
- checkNotNull(getTerminationPerInput(), "terminationPerInput");
+ checkArgumentNotNull(getPollInterval(), "pollInterval");
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/SerializableAvroCodecFactory.java:
##########
@@ -101,12 +106,15 @@ public void readExternal(ObjectInput in) throws
IOException, ClassNotFoundExcept
}
public CodecFactory getCodec() {
+
return codecFactory;
}
@Override
public String toString() {
- checkNotNull(codecFactory, "Inner CodecFactory is null, please use non
default constructor");
+
+ checkArgumentNotNull(
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/options/ValueProvider.java:
##########
@@ -357,7 +358,7 @@ public ValueProvider<?> deserialize(JsonParser jp,
DeserializationContext ctxt)
throws IOException, JsonProcessingException {
JsonDeserializer dser =
ctxt.findRootValueDeserializer(
- checkNotNull(
+ checkArgumentNotNull(
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/MapKeys.java:
##########
@@ -154,7 +154,7 @@ public PCollection<KV<K2, V>> expand(PCollection<KV<K1, V>>
input) {
return input.apply(
"MapKeys",
MapElements.into(getKvTypeDescriptor())
- .via(checkNotNull(fn, "Must specify a function on MapKeys using
.via()")));
+ .via(checkArgumentNotNull(fn, "Must specify a function on MapKeys
using .via()")));
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/MapElements.java:
##########
@@ -130,7 +130,7 @@ public <NewInputT> MapElements<NewInputT, OutputT>
via(Contextful<Fn<NewInputT,
@Override
public PCollection<OutputT> expand(PCollection<? extends InputT> input) {
- checkNotNull(fn, "Must specify a function on MapElements using .via()");
+ checkArgumentNotNull(fn, "Must specify a function on MapElements using
.via()");
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/TextIO.java:
##########
@@ -397,7 +397,8 @@ static boolean isSelfOverlapping(byte[] s) {
@Override
public PCollection<String> expand(PBegin input) {
- checkNotNull(getFilepattern(), "need to set the filepattern of a
TextIO.Read transform");
+ checkArgumentNotNull(
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/io/Read.java:
##########
@@ -525,8 +525,8 @@ public void splitRestriction(
restrictionTracker(
@Restriction UnboundedSourceRestriction<OutputT, CheckpointT>
restriction,
PipelineOptions pipelineOptions) {
- checkNotNull(restrictionCoder);
- checkNotNull(cachedReaders);
+ checkArgumentNotNull(restrictionCoder);
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/test/java/org/apache/beam/sdk/transforms/CombineTest.java:
##########
@@ -342,7 +342,7 @@ public Accumulator mergeAccumulators(Iterable<Accumulator>
accumulators) {
all.append(accumulator.value);
accumulator.value = "cleared in mergeAccumulators";
}
- return new Accumulator(checkNotNull(seedAccumulator).seed,
all.toString());
+ return new Accumulator(checkArgumentNotNull(seedAccumulator).seed,
all.toString());
Review Comment:
`checkStateNotNull`
##########
sdks/java/core/src/main/java/org/apache/beam/sdk/transforms/MapValues.java:
##########
@@ -155,7 +155,7 @@ public PCollection<KV<K, V2>> expand(PCollection<KV<K, V1>>
input) {
return input.apply(
"MapValues",
MapElements.into(getKvTypeDescriptor())
- .via(checkNotNull(fn, "Must specify a function on MapValues using
.via()")));
+ .via(checkArgumentNotNull(fn, "Must specify a function on
MapValues using .via()")));
Review Comment:
`checkStateNotNull`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
