damondouglas commented on code in PR #32367: URL: https://github.com/apache/beam/pull/32367#discussion_r1755272531
########## sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java: ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.sdk.util; + +import java.io.Serializable; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.beam.sdk.schemas.Schema; +import org.apache.beam.sdk.transforms.windowing.BoundedWindow; +import org.apache.beam.sdk.transforms.windowing.PaneInfo; +import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.MoreObjects; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.joda.time.Instant; + +/** + * A utility that interpolates values in a pre-determined {@link String} using an input Beam {@link + * Row}. + * + * <p>The {@link RowStringInterpolator} looks for field names specified inside {curly braces}. For + * example, if the interpolator is configured with the String {@code "unified {foo} and streaming"}, + * it will look for a field name {@code "foo"} in the input {@link Row} and substitute in that + * value. If a {@link RowStringInterpolator} is configured with a template String that contains no + * placeholders (i.e. no curly braces), it will simply return that String, untouched. + * + * <p>Nested fields can be specified using dot-notation (e.g. {@code "top.middle.nested"}). + * + * <p>Configure a {@link RowStringInterpolator} like so: + * + * <pre>{@code + * String template = "unified {foo} and {bar.baz}!"; + * Row inputRow = {foo: "batch", bar: {baz: "streaming"}, ...}; + * + * RowStringInterpolator interpolator = new RowStringInterpolator(template, beamSchema); + * String output = interpolator.interpolate(inputRow); + * // output --> "unified batch and streaming!" + * }</pre> + * + * <p>Additionally, {@link #interpolate(Row, BoundedWindow, PaneInfo, Instant)} can be used in + * streaming scenarios to substitute windowing metadata into the template String. To make use of + * this, use the relevant placeholder: + * + * <ul> + * <li>$WINDOW: the window's string representation + * <li>$PANE_INDEX: the pane's index + * <li>$YYYY: the element timestamp's year + * <li>$MM: the element timestamp's month + * <li>$DD: the element timestamp's day + * </ul> + * + * <p>For example, your Sting template can look like: + * + * <pre>{@code "unified {foo} and {bar} since {$YYYY}-{$MM}!"}</pre> + */ +public class RowStringInterpolator implements Serializable { + private final String template; + private final Set<String> fieldsToReplace; + public static final String WINDOW = "$WINDOW"; + public static final String PANE_INDEX = "$PANE_INDEX"; + public static final String YYYY = "$YYYY"; + public static final String MM = "$MM"; + public static final String DD = "$DD"; Review Comment: May we consider adding code comments to these. The date parts `YYYY`, etc may be obvious but possibly not the `WINDOW` or `PANE_INDEX`. ########## sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java: ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.sdk.util; + +import java.io.Serializable; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.beam.sdk.schemas.Schema; +import org.apache.beam.sdk.transforms.windowing.BoundedWindow; +import org.apache.beam.sdk.transforms.windowing.PaneInfo; +import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.MoreObjects; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.joda.time.Instant; + +/** + * A utility that interpolates values in a pre-determined {@link String} using an input Beam {@link + * Row}. + * + * <p>The {@link RowStringInterpolator} looks for field names specified inside {curly braces}. For + * example, if the interpolator is configured with the String {@code "unified {foo} and streaming"}, + * it will look for a field name {@code "foo"} in the input {@link Row} and substitute in that + * value. If a {@link RowStringInterpolator} is configured with a template String that contains no + * placeholders (i.e. no curly braces), it will simply return that String, untouched. + * + * <p>Nested fields can be specified using dot-notation (e.g. {@code "top.middle.nested"}). + * + * <p>Configure a {@link RowStringInterpolator} like so: + * + * <pre>{@code + * String template = "unified {foo} and {bar.baz}!"; + * Row inputRow = {foo: "batch", bar: {baz: "streaming"}, ...}; + * + * RowStringInterpolator interpolator = new RowStringInterpolator(template, beamSchema); + * String output = interpolator.interpolate(inputRow); + * // output --> "unified batch and streaming!" + * }</pre> + * + * <p>Additionally, {@link #interpolate(Row, BoundedWindow, PaneInfo, Instant)} can be used in + * streaming scenarios to substitute windowing metadata into the template String. To make use of + * this, use the relevant placeholder: + * + * <ul> + * <li>$WINDOW: the window's string representation + * <li>$PANE_INDEX: the pane's index + * <li>$YYYY: the element timestamp's year + * <li>$MM: the element timestamp's month + * <li>$DD: the element timestamp's day + * </ul> + * + * <p>For example, your Sting template can look like: + * + * <pre>{@code "unified {foo} and {bar} since {$YYYY}-{$MM}!"}</pre> + */ +public class RowStringInterpolator implements Serializable { + private final String template; + private final Set<String> fieldsToReplace; + public static final String WINDOW = "$WINDOW"; + public static final String PANE_INDEX = "$PANE_INDEX"; + public static final String YYYY = "$YYYY"; + public static final String MM = "$MM"; + public static final String DD = "$DD"; + private static final Set<String> WINDOWING_METADATA = + Sets.newHashSet(WINDOW, PANE_INDEX, YYYY, MM, DD); + + public RowStringInterpolator(String template, Schema rowSchema) { Review Comment: I'll leave it for you to judge. May we consider a code comment that at least refers the developer to the class level documentation. Schema might be obvious but the String template may not be unless I read the top level documentation. ########## sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java: ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.sdk.util; + +import java.io.Serializable; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.beam.sdk.schemas.Schema; +import org.apache.beam.sdk.transforms.windowing.BoundedWindow; +import org.apache.beam.sdk.transforms.windowing.PaneInfo; +import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.MoreObjects; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.joda.time.Instant; + +/** + * A utility that interpolates values in a pre-determined {@link String} using an input Beam {@link + * Row}. + * + * <p>The {@link RowStringInterpolator} looks for field names specified inside {curly braces}. For + * example, if the interpolator is configured with the String {@code "unified {foo} and streaming"}, + * it will look for a field name {@code "foo"} in the input {@link Row} and substitute in that + * value. If a {@link RowStringInterpolator} is configured with a template String that contains no + * placeholders (i.e. no curly braces), it will simply return that String, untouched. + * + * <p>Nested fields can be specified using dot-notation (e.g. {@code "top.middle.nested"}). + * + * <p>Configure a {@link RowStringInterpolator} like so: + * + * <pre>{@code + * String template = "unified {foo} and {bar.baz}!"; + * Row inputRow = {foo: "batch", bar: {baz: "streaming"}, ...}; + * + * RowStringInterpolator interpolator = new RowStringInterpolator(template, beamSchema); + * String output = interpolator.interpolate(inputRow); + * // output --> "unified batch and streaming!" + * }</pre> + * + * <p>Additionally, {@link #interpolate(Row, BoundedWindow, PaneInfo, Instant)} can be used in + * streaming scenarios to substitute windowing metadata into the template String. To make use of + * this, use the relevant placeholder: + * + * <ul> + * <li>$WINDOW: the window's string representation + * <li>$PANE_INDEX: the pane's index + * <li>$YYYY: the element timestamp's year + * <li>$MM: the element timestamp's month + * <li>$DD: the element timestamp's day + * </ul> + * + * <p>For example, your Sting template can look like: + * + * <pre>{@code "unified {foo} and {bar} since {$YYYY}-{$MM}!"}</pre> + */ +public class RowStringInterpolator implements Serializable { + private final String template; + private final Set<String> fieldsToReplace; + public static final String WINDOW = "$WINDOW"; + public static final String PANE_INDEX = "$PANE_INDEX"; + public static final String YYYY = "$YYYY"; + public static final String MM = "$MM"; + public static final String DD = "$DD"; + private static final Set<String> WINDOWING_METADATA = + Sets.newHashSet(WINDOW, PANE_INDEX, YYYY, MM, DD); + + public RowStringInterpolator(String template, Schema rowSchema) { + this.template = template; + + Matcher m = Pattern.compile("\\{(.+?)}").matcher(template); Review Comment: May we consider a static final Pattern, where the Matcher is instantiated from using the constructor provided template? ########## sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java: ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.sdk.util; + +import java.io.Serializable; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.beam.sdk.schemas.Schema; +import org.apache.beam.sdk.transforms.windowing.BoundedWindow; +import org.apache.beam.sdk.transforms.windowing.PaneInfo; +import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.MoreObjects; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.joda.time.Instant; + +/** + * A utility that interpolates values in a pre-determined {@link String} using an input Beam {@link + * Row}. + * + * <p>The {@link RowStringInterpolator} looks for field names specified inside {curly braces}. For + * example, if the interpolator is configured with the String {@code "unified {foo} and streaming"}, + * it will look for a field name {@code "foo"} in the input {@link Row} and substitute in that + * value. If a {@link RowStringInterpolator} is configured with a template String that contains no + * placeholders (i.e. no curly braces), it will simply return that String, untouched. + * + * <p>Nested fields can be specified using dot-notation (e.g. {@code "top.middle.nested"}). + * + * <p>Configure a {@link RowStringInterpolator} like so: + * + * <pre>{@code + * String template = "unified {foo} and {bar.baz}!"; + * Row inputRow = {foo: "batch", bar: {baz: "streaming"}, ...}; + * + * RowStringInterpolator interpolator = new RowStringInterpolator(template, beamSchema); + * String output = interpolator.interpolate(inputRow); + * // output --> "unified batch and streaming!" + * }</pre> + * + * <p>Additionally, {@link #interpolate(Row, BoundedWindow, PaneInfo, Instant)} can be used in + * streaming scenarios to substitute windowing metadata into the template String. To make use of + * this, use the relevant placeholder: + * + * <ul> + * <li>$WINDOW: the window's string representation + * <li>$PANE_INDEX: the pane's index + * <li>$YYYY: the element timestamp's year + * <li>$MM: the element timestamp's month + * <li>$DD: the element timestamp's day + * </ul> + * + * <p>For example, your Sting template can look like: + * + * <pre>{@code "unified {foo} and {bar} since {$YYYY}-{$MM}!"}</pre> + */ +public class RowStringInterpolator implements Serializable { + private final String template; + private final Set<String> fieldsToReplace; + public static final String WINDOW = "$WINDOW"; + public static final String PANE_INDEX = "$PANE_INDEX"; + public static final String YYYY = "$YYYY"; + public static final String MM = "$MM"; + public static final String DD = "$DD"; + private static final Set<String> WINDOWING_METADATA = + Sets.newHashSet(WINDOW, PANE_INDEX, YYYY, MM, DD); + + public RowStringInterpolator(String template, Schema rowSchema) { + this.template = template; + + Matcher m = Pattern.compile("\\{(.+?)}").matcher(template); + fieldsToReplace = new HashSet<>(); + while (m.find()) { + fieldsToReplace.add(StringUtils.strip(m.group(), "{}")); + } + + List<String> rowFields = + fieldsToReplace.stream() + .filter(f -> !WINDOWING_METADATA.contains(f)) + .collect(Collectors.toList()); + + RowFilter.validateSchemaContainsFields(rowSchema, rowFields, "string interpolation"); + } + + /** Performs string interpolation on the template using values from the input {@link Row}. */ + public String interpolate(Row row) { + String interpolated = this.template; + for (String field : fieldsToReplace) { + List<String> levels = Splitter.on(".").splitToList(field); + + Object val = MoreObjects.firstNonNull(getValue(row, levels, 0), ""); + + interpolated = interpolated.replace("{" + field + "}", String.valueOf(val)); + } + return interpolated; + } + + /** Like {@link #interpolate(Row)} but also potentially include windowing information. */ + public String interpolate(Row row, BoundedWindow window, PaneInfo paneInfo, Instant timestamp) { Review Comment: Non blocking but I was concerned about the possibility for future new sources of interpolation. For example, what if someone wants to add a new Thing but include the existing sources of interpolate, row, window, etc? Then in another future, someone else wants to add a new Thing2. This may require some thought. However, what I was envisioning was something like this: ``` interface Interpolator { String interpolate(); } // instantiated with a helper method maybe class RowInterpolator { String interpolate() { } } class RowStringInterpolator public String interpolate(Interpolator ...interpolators) { ... } } String result = interpolator.interpolate(rowInterpolator, windowInterpolator, paneInfoInterpolator, timestampInterpolator); ``` ########## sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java: ########## @@ -0,0 +1,181 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.beam.sdk.util; + +import java.io.Serializable; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import java.util.stream.Collectors; +import org.apache.beam.sdk.schemas.Schema; +import org.apache.beam.sdk.transforms.windowing.BoundedWindow; +import org.apache.beam.sdk.transforms.windowing.PaneInfo; +import org.apache.beam.sdk.values.Row; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.MoreObjects; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Preconditions; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.base.Splitter; +import org.apache.beam.vendor.guava.v32_1_2_jre.com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; +import org.checkerframework.checker.nullness.qual.Nullable; +import org.joda.time.Instant; + +/** + * A utility that interpolates values in a pre-determined {@link String} using an input Beam {@link + * Row}. + * + * <p>The {@link RowStringInterpolator} looks for field names specified inside {curly braces}. For + * example, if the interpolator is configured with the String {@code "unified {foo} and streaming"}, + * it will look for a field name {@code "foo"} in the input {@link Row} and substitute in that + * value. If a {@link RowStringInterpolator} is configured with a template String that contains no + * placeholders (i.e. no curly braces), it will simply return that String, untouched. + * + * <p>Nested fields can be specified using dot-notation (e.g. {@code "top.middle.nested"}). + * + * <p>Configure a {@link RowStringInterpolator} like so: + * + * <pre>{@code + * String template = "unified {foo} and {bar.baz}!"; + * Row inputRow = {foo: "batch", bar: {baz: "streaming"}, ...}; + * + * RowStringInterpolator interpolator = new RowStringInterpolator(template, beamSchema); + * String output = interpolator.interpolate(inputRow); + * // output --> "unified batch and streaming!" + * }</pre> + * + * <p>Additionally, {@link #interpolate(Row, BoundedWindow, PaneInfo, Instant)} can be used in + * streaming scenarios to substitute windowing metadata into the template String. To make use of + * this, use the relevant placeholder: + * + * <ul> + * <li>$WINDOW: the window's string representation + * <li>$PANE_INDEX: the pane's index + * <li>$YYYY: the element timestamp's year + * <li>$MM: the element timestamp's month + * <li>$DD: the element timestamp's day + * </ul> + * + * <p>For example, your Sting template can look like: + * + * <pre>{@code "unified {foo} and {bar} since {$YYYY}-{$MM}!"}</pre> + */ +public class RowStringInterpolator implements Serializable { + private final String template; + private final Set<String> fieldsToReplace; + public static final String WINDOW = "$WINDOW"; + public static final String PANE_INDEX = "$PANE_INDEX"; + public static final String YYYY = "$YYYY"; + public static final String MM = "$MM"; + public static final String DD = "$DD"; + private static final Set<String> WINDOWING_METADATA = + Sets.newHashSet(WINDOW, PANE_INDEX, YYYY, MM, DD); + + public RowStringInterpolator(String template, Schema rowSchema) { + this.template = template; + + Matcher m = Pattern.compile("\\{(.+?)}").matcher(template); + fieldsToReplace = new HashSet<>(); + while (m.find()) { + fieldsToReplace.add(StringUtils.strip(m.group(), "{}")); + } + + List<String> rowFields = + fieldsToReplace.stream() + .filter(f -> !WINDOWING_METADATA.contains(f)) + .collect(Collectors.toList()); + + RowFilter.validateSchemaContainsFields(rowSchema, rowFields, "string interpolation"); Review Comment: The following results from my concern for doing work inside a constructor. I don't want to block this PR. I will move on if you resolve this comment. However, brainstorming out loud, may we consider a constructor, possibly package private, that accepts the template and fieldsToReplace. Then provide a static method, perhaps called compile, that performs the work of this constructor. The benefits of this is that testing the interpolate could be independent of testing the compile phase. Also, this separation may make changes in the future possibly easier as they seem like separate but related phases. -- 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]
