gemini-code-assist[bot] commented on code in PR #37014:
URL: https://github.com/apache/beam/pull/37014#discussion_r2593818655
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/DynamicDestinationsHelpers.java:
##########
@@ -279,11 +279,17 @@ static class
ConstantTimePartitioningClusteringDestinations<T>
private final @Nullable ValueProvider<String> jsonTimePartitioning;
private final @Nullable ValueProvider<String> jsonClustering;
+ // Lazily initialized and cached values.
+ private @Nullable String evaluatedPartitioning = null;
+ private @Nullable String evaluatedClustering = null;
+ private AtomicBoolean initialized = null;
+
ConstantTimePartitioningClusteringDestinations(
DynamicDestinations<T, TableDestination> inner,
ValueProvider<String> jsonTimePartitioning,
ValueProvider<String> jsonClustering) {
super(inner);
+ initialized = new AtomicBoolean(false);
Review Comment:
For thread-safety and to follow best practices, the `initialized` flag
should be declared as `final` to ensure it is set only once during object
construction and its value is safely published to other threads. You can
initialize it at the declaration and remove the redundant initialization from
the constructor.
```suggestion
private final AtomicBoolean initialized = new AtomicBoolean(false);
ConstantTimePartitioningClusteringDestinations(
DynamicDestinations<T, TableDestination> inner,
ValueProvider<String> jsonTimePartitioning,
ValueProvider<String> jsonClustering) {
super(inner);
```
##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigquery/DynamicDestinationsHelpers.java:
##########
@@ -299,19 +305,36 @@ static class
ConstantTimePartitioningClusteringDestinations<T>
this.jsonClustering = jsonClustering;
}
+ private synchronized void evaluateOncePartitioningAndClustering() {
+ if (initialized.get()) {
+ return;
+ }
+ if (jsonTimePartitioning != null) {
+ String partitioning = jsonTimePartitioning.get();
+ if (partitioning != null
+ &&
!JsonParser.parseString(partitioning).getAsJsonObject().isEmpty()) {
+ this.evaluatedPartitioning = partitioning;
+ }
+ }
+ if (jsonClustering != null) {
+ String clustering = jsonClustering.get();
+ if (clustering != null &&
!JsonParser.parseString(clustering).getAsJsonObject().isEmpty()) {
+ this.evaluatedClustering = clustering;
Review Comment:
The logic to check if the JSON string is present and represents a non-empty
object is duplicated for both `partitioning` and `clustering`. To improve
maintainability and reduce code duplication, consider extracting this check
into a private helper method, for example `isJsonConfigPresent(String json)`.
--
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]
