gemini-code-assist[bot] commented on code in PR #38341:
URL: https://github.com/apache/beam/pull/38341#discussion_r3168898676
##########
examples/java/src/main/java/org/apache/beam/examples/KafkaStreaming.java:
##########
@@ -293,17 +290,17 @@ public Map<String, Integer> extractOutput(Map<String,
Integer> accumulator) {
static class LogResults extends DoFn<Map<String, Integer>, Map<String,
Integer>> {
@ProcessElement
- public void processElement(
- PaneInfo pane,
- IntervalWindow w,
- @Element Map<String, Integer> scores,
- OutputReceiver<Map<String, Integer>> receiver)
- throws Exception {
+ public void processElement(ProcessContext c, IntervalWindow w) throws
Exception {
+ Map<String, Integer> map = c.element();
+ if (map == null) {
+ c.output(c.element());
+ return;
+ }
Review Comment:
The logic here is redundant and potentially problematic. If `map` is null,
calling `c.output(c.element())` will output a null value, which might cause
issues with downstream transforms or coders. If the intention is to skip null
elements, the method should simply return. If the intention is to pass them
through, it is more efficient to use the local variable `map` instead of
calling `c.element()` again.
```java
if (map == null) {
return;
}
```
##########
examples/java/src/main/java/org/apache/beam/examples/KafkaPassengerCountJson.java:
##########
@@ -129,11 +124,11 @@ public void processElement(
new DoFn<KV<Integer, Integer>, KV<Integer, Integer>>() {
@ProcessElement
public void processElement(
- OutputReceiver<KV<Integer, Integer>> out,
- @Element KV<Integer, Integer> element) {
+ ProcessContext c, OutputReceiver<KV<Integer, Integer>>
out) {
System.out.printf(
- "Vendor: %s, Passengers: %s%n", element.getKey(),
element.getValue());
- out.output(element);
+ "Vendor: %s, Passengers: %s%n",
+ c.element().getKey(), c.element().getValue());
+ out.output(c.element());
Review Comment:
This method signature mixes legacy `ProcessContext` with the modern
`OutputReceiver` parameter. For consistency and clarity, it is recommended to
use one style. Since this PR is reverting to `ProcessContext`, you should use
`c.output()` instead of the `out` parameter.
```suggestion
ProcessContext c) {
System.out.printf(
"Vendor: %s, Passengers: %s%n",
c.element().getKey(), c.element().getValue());
c.output(c.element());
```
##########
examples/java/src/main/java/org/apache/beam/examples/complete/datatokenization/transforms/DataProtectors.java:
##########
@@ -202,24 +199,20 @@ public void close() {
@ProcessElement
@SuppressWarnings("argument")
- public void process(
- @Element KV<Integer, Iterable<Row>> element,
- OutputReceiver<Row> mainReceiver,
- MultiOutputReceiver multiReceiver) {
+ public void process(@Element KV<Integer, Iterable<Row>> element,
ProcessContext context) {
Iterable<Row> rows = element.getValue();
Review Comment:
Mixing `@Element` parameter injection with `ProcessContext` is redundant and
inconsistent. Since `ProcessContext` is being used for output later in the
method, it should also be used to access the element via `context.element()`
for a consistent API usage style.
```suggestion
public void process(ProcessContext context) {
Iterable<Row> rows = context.element().getValue();
```
##########
examples/java/src/main/java/org/apache/beam/examples/complete/datatokenization/utils/CsvConverters.java:
##########
@@ -552,12 +510,13 @@ static class GetCsvHeadersFn extends DoFn<ReadableFile,
String> {
}
@ProcessElement
- public void processElement(@Element ReadableFile file, MultiOutputReceiver
outputReceiver) {
+ public void processElement(ProcessContext context, MultiOutputReceiver
outputReceiver) {
Review Comment:
This method signature contains both `ProcessContext` and
`MultiOutputReceiver`. This is redundant because `ProcessContext` already
provides the ability to output to multiple tags via `context.output(tag,
value)`. For consistency, especially in a PR reverting to legacy context usage,
the `MultiOutputReceiver` parameter should be removed.
```suggestion
public void processElement(ProcessContext context) {
```
##########
examples/java/src/main/java/org/apache/beam/examples/complete/datatokenization/transforms/io/TokenizationBigTableIO.java:
##########
@@ -75,10 +75,8 @@ static class TransformToBigTableFormat extends DoFn<Row,
KV<ByteString, Iterable
@ProcessElement
public void processElement(
- @Element Row in,
- OutputReceiver<KV<ByteString, Iterable<Mutation>>> out,
- PipelineOptions pipelineOptions) {
- DataTokenizationOptions options =
pipelineOptions.as(DataTokenizationOptions.class);
+ @Element Row in, OutputReceiver<KV<ByteString, Iterable<Mutation>>>
out, ProcessContext c) {
+ DataTokenizationOptions options =
c.getPipelineOptions().as(DataTokenizationOptions.class);
Review Comment:
The `ProcessContext` parameter is only used here to access
`PipelineOptions`. In Apache Beam, `PipelineOptions` can be injected directly
as a parameter to `@ProcessElement`. Mixing `ProcessContext` with modern
parameters like `@Element` and `OutputReceiver` is discouraged as it makes the
method signature unnecessarily complex.
```java
public void processElement(
@Element Row in,
OutputReceiver<KV<ByteString, Iterable<Mutation>>> out,
PipelineOptions pipelineOptions) {
DataTokenizationOptions options =
pipelineOptions.as(DataTokenizationOptions.class);
```
--
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]
