Add RowStringInterpolator utility #32367

ahmedabu98 · 2024-08-29T19:28:07Z

Utility to substitute values from Beam Rows into a template String.
Depends on #32366

Part of #32365
See #31807 for how this can work for portable dynamic destinations

github-actions · 2024-08-29T20:06:10Z

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

ahmedabu98 · 2024-08-30T13:00:37Z

assign set of reviewers

ahmedabu98 · 2024-08-30T13:01:01Z

tests are red because #32366 hasn't been merged yet, but this PR is still ready for review

github-actions · 2024-08-30T13:01:50Z

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @damondouglas for label java.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

github-actions · 2024-09-07T12:13:51Z

Reminder, please take a look at this pr: @damondouglas

github-actions · 2024-09-11T12:14:14Z

Assigning new set of reviewers because Pr has gone too long without review. If you would like to opt out of this review, comment assign to next reviewer:

R: @robertwb for label java.

Available commands:

stop reviewer notifications - opt out of the automated review tooling
remind me after tests pass - tag the comment author after tests pass
waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

damondouglas · 2024-09-11T18:32:09Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+    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");


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.

I'd prefer if it was not possible for one to instantiate an invalid interpolator

+1 to not being able to instantiate an invalid interpolator.

damondouglas · 2024-09-11T18:42:59Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+  }
+
+  /** Like {@link #interpolate(Row)} but also potentially include windowing information. */
+  public String interpolate(Row row, BoundedWindow window, PaneInfo paneInfo, Instant timestamp) {


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);

Yeah I could see this being something like

interface Interpolator<T> { String interpolate(T); ... } RowStringInterpolator implements Interpolator<Row> {}

With that said though, I'd prefer we leave it off until it is warranted. Most of this work is just for cross-language and portability, where Beam Row is used as a standard. I haven't come across a case where we would benefit with String interpolation using another type, so modularizing this may not actually make a difference.

It's possible that we would want to support "row-compatible" objects, not just Rows, but I don't think that needs to be done from the start.

ahmedabu98 · 2024-09-15T20:46:04Z

Thanks @damondouglas, PTAL

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

robertwb · 2024-09-17T23:53:33Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+    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");


+1 to not being able to instantiate an invalid interpolator.

robertwb · 2024-09-17T23:55:08Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+  public static final String DD = "$DD";
+  private static final Set<String> WINDOWING_METADATA =
+      Sets.newHashSet(WINDOW, PANE_INDEX, YYYY, MM, DD);
+  private static final Pattern TEMPLATE_PATTERN = Pattern.compile("\\{(.+?)}");


Do we not care about being able to escape a {? (Maybe this is fine as future work.)

It complains if we don't escape it in the pattern:

java.util.regex.PatternSyntaxException: Illegal repetition {(.+?)}

Normally, curly brackets signify how many times a character should be repeated

robertwb · 2024-09-17T23:57:19Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+  }
+
+  /** Like {@link #interpolate(Row)} but also potentially include windowing information. */
+  public String interpolate(Row row, BoundedWindow window, PaneInfo paneInfo, Instant timestamp) {


It's possible that we would want to support "row-compatible" objects, not just Rows, but I don't think that needs to be done from the start.

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

robertwb · 2024-09-18T00:06:58Z

sdks/java/core/src/main/java/org/apache/beam/sdk/util/RowStringInterpolator.java

+  }
+
+  /** Performs string interpolation on the template using values from the input {@link Row}. */
+  public String interpolate(Row row) {


What is the motivation to have a separate no-windowing implementation?

Figured why not, but on a second thought will remove it to keep windowing properties as first-class citizens

…interpolator

* RowStringInterpolator * spotless * address comments * address comments

RowStringInterpolator

d24f39d

github-actions bot added the java label Aug 29, 2024

spotless

b178477

github-actions bot added the Next Action: Reviewers label Aug 30, 2024

github-actions bot added the slow-review label Sep 7, 2024

github-actions bot removed the slow-review label Sep 11, 2024

damondouglas self-requested a review September 11, 2024 18:07

damondouglas requested changes Sep 11, 2024

View reviewed changes

address comments

3a9aee0

robertwb approved these changes Sep 18, 2024

View reviewed changes

ahmedabu98 added 3 commits September 18, 2024 15:40

address comments

4abf0aa

Merge branch 'master' of https://github.com/ahmedabu98/beam into row_…

3c0e9e9

…interpolator

Merge branch 'master' of https://github.com/ahmedabu98/beam into row_…

20598c2

…interpolator

ahmedabu98 merged commit e3c6f47 into apache:master Sep 25, 2024
28 checks passed

reeba212 pushed a commit to reeba212/beam that referenced this pull request Dec 4, 2024

Add RowStringInterpolator utility (apache#32367)

f646c7c

* RowStringInterpolator * spotless * address comments * address comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add RowStringInterpolator utility #32367

Add RowStringInterpolator utility #32367

ahmedabu98 commented Aug 29, 2024

github-actions bot commented Aug 29, 2024

ahmedabu98 commented Aug 30, 2024

ahmedabu98 commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

github-actions bot commented Sep 7, 2024

github-actions bot commented Sep 11, 2024

damondouglas Sep 11, 2024

ahmedabu98 Sep 15, 2024

robertwb Sep 17, 2024

damondouglas Sep 11, 2024

ahmedabu98 Sep 15, 2024

robertwb Sep 17, 2024

ahmedabu98 commented Sep 15, 2024

robertwb Sep 17, 2024

robertwb Sep 17, 2024

ahmedabu98 Sep 18, 2024

robertwb Sep 17, 2024

robertwb Sep 18, 2024

ahmedabu98 Sep 18, 2024

Add RowStringInterpolator utility #32367

Add RowStringInterpolator utility #32367

Conversation

ahmedabu98 commented Aug 29, 2024

github-actions bot commented Aug 29, 2024

ahmedabu98 commented Aug 30, 2024

ahmedabu98 commented Aug 30, 2024

github-actions bot commented Aug 30, 2024

github-actions bot commented Sep 7, 2024

github-actions bot commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahmedabu98 commented Sep 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment