You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@beam.apache.org by GitBox <gi...@apache.org> on 2022/11/22 04:04:14 UTC

[GitHub] [beam] camphillips22 opened a new pull request, #24307: Add map_windows support to Go SDK

camphillips22 opened a new pull request, #24307:
URL: https://github.com/apache/beam/pull/24307

   Adds support to the Go SDK for the [map_windows][1] urn.
   
   The existing type model does not allow `IntervalWindow` as a FullValue and the existing coders will throw away the window values when decoding the KV type [here][2] in `elideSingleElmFV`. This change updates coder construction to allow an IntervalWindow as a value coder.
   
   Addresses #23106.
   
   [1]: https://github.com/apache/beam/blob/master/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto#L296
   [2]: https://github.com/camphillips22/beam/blob/8be4cdcaf65a1e53e3041ac3354e2e99c845e915/sdks/go/pkg/beam/core/runtime/exec/coder.go#L580-L580
   
   ------------------------
   
   Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
   
    - [ ] [**Choose reviewer(s)**](https://beam.apache.org/contribute/#make-your-change) and mention them in a comment (`R: @username`).
    - [ ] Mention the appropriate issue in your description (for example: `addresses #123`), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment `fixes #<ISSUE NUMBER>` instead.
    - [ ] Update `CHANGES.md` with noteworthy changes.
    - [ ] If this contribution is large, please file an Apache [Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
   
   See the [Contributor Guide](https://beam.apache.org/contribute) for more tips on [how to make review process smoother](https://beam.apache.org/contribute/get-started-contributing/#make-the-reviewers-job-easier).
   
   To check the build health, please visit [https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md](https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md)
   
   GitHub Actions Tests Status (on master branch)
   ------------------------------------------------------------------------------------------------
   [![Build python source distribution and wheels](https://github.com/apache/beam/workflows/Build%20python%20source%20distribution%20and%20wheels/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Build+python+source+distribution+and+wheels%22+branch%3Amaster+event%3Aschedule)
   [![Python tests](https://github.com/apache/beam/workflows/Python%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Python+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Java tests](https://github.com/apache/beam/workflows/Java%20Tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Java+Tests%22+branch%3Amaster+event%3Aschedule)
   [![Go tests](https://github.com/apache/beam/workflows/Go%20tests/badge.svg?branch=master&event=schedule)](https://github.com/apache/beam/actions?query=workflow%3A%22Go+tests%22+branch%3Amaster+event%3Aschedule)
   
   See [CI.md](https://github.com/apache/beam/blob/master/CI.md) for more information about GitHub Actions CI.
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1323722704

   A note about testing: I was unable to write a batch pipeline that resulted in a window mapping operation. 
   
   [Here](https://github.com/camphillips22/beam/compare/cam/mapwindows...camphillips22:beam:mapwindows-manual-tests?expand=1) is a link to the tests I wrote which did not result in a map_windows operation. It also contains the changes I made to the streaming wordcap example to make it happen on dataflow. I have not written a pipeline yet that works on Flink and uses map windows, but I wanted to go ahead get this change out for review.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030957657


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -379,18 +379,14 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 			return nil, err
 		}
 		return coder.NewN(elm), nil
-
-		// Special handling for window coders so they can be treated as
-		// a general coder. Generally window coders are not used outside of
-		// specific contexts, but this enables improved testing.
-		// Window types are not permitted to be fulltypes, so
-		// we use assignably equivalent anonymous struct types.
 	case urnIntervalWindow:

Review Comment:
   I think that's the only remaining thing for me to block on before merging.
   
   Also, FYI, my foretold change: #24346 
   At this point I'll handle the merge conflict instead.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331497475

   Thank you very much for working through this! Hopefully now it's possible for someone to cobble together a PeriodicImpulse or similar and have functioning Slowly Changing Side Inputs.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1323020311

   R: @lostluck 


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331309234

   Run Go PreCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1033609188


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -379,18 +379,14 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 			return nil, err
 		}
 		return coder.NewN(elm), nil
-
-		// Special handling for window coders so they can be treated as
-		// a general coder. Generally window coders are not used outside of
-		// specific contexts, but this enables improved testing.
-		// Window types are not permitted to be fulltypes, so
-		// we use assignably equivalent anonymous struct types.
 	case urnIntervalWindow:

Review Comment:
   Done!



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331314202

   Run Go PostCommit


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331312297

   PRecommit failure was a flake, so just rerunning to be sure. Otherwise this LGTM.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030686917


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 				t := typex.New(root, append([]typex.FullType{key.T}, coder.Types(values)...)...)
 				return &coder.Coder{Kind: kind, T: t, Components: append([]*coder.Coder{key}, values...)}, nil
 			}
+		case urnIntervalWindow:
+			// If interval window in a KV, this may be a mapping function.
+			// Special case since windows are not normally used directly as FullValues.
+			return coder.NewIntervalWindowCoder(), nil

Review Comment:
   > It'd be better if we follow the normal structure, generalizing to handle interval windows better instead of special casing here.
   
   To clarify, are you suggesting that we need to pull this special case out of the KV coder case, or just to return an actual KV coder?



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030711314


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -248,7 +248,9 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 		case urnIntervalWindow:
 			// If interval window in a KV, this may be a mapping function.
 			// Special case since windows are not normally used directly as FullValues.
-			return coder.NewIntervalWindowCoder(), nil
+			value := coder.NewIntervalWindowCoder()
+			t := typex.New(root, key.T, value.T)
+			return &coder.Coder{Kind: kind, T: t, Components: []*coder.Coder{key, value}}, nil

Review Comment:
   Ah didn't see this. Basically, we should only need to put the `case urnIntervalWindow: return coder.NewIntervalWindowCoder(), nil` into the outer switch (probably just above KV, since it's easier to read), and not explicitly handle it under the KV coder case.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030741894


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -248,7 +248,9 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 		case urnIntervalWindow:
 			// If interval window in a KV, this may be a mapping function.
 			// Special case since windows are not normally used directly as FullValues.
-			return coder.NewIntervalWindowCoder(), nil
+			value := coder.NewIntervalWindowCoder()
+			t := typex.New(root, key.T, value.T)
+			return &coder.Coder{Kind: kind, T: t, Components: []*coder.Coder{key, value}}, nil

Review Comment:
   As the person who put that in, I'd rather inconvenience the tests than the production code. Please feel free to change them up. (sorry for the churn).
   That was written before I was aware that there were uses for runner sending coders around like that, and we just wanted to validate the coders per the standard_coders.yaml tests.
   
   Those uses are handled here usually: https://github.com/camphillips22/beam/tree/c83b42f7547a5fd57e2f9a90d7b613c512eb088c/sdks/go/test/regression/coders/fromyaml



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030708807


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 				t := typex.New(root, append([]typex.FullType{key.T}, coder.Types(values)...)...)
 				return &coder.Coder{Kind: kind, T: t, Components: append([]*coder.Coder{key}, values...)}, nil
 			}
+		case urnIntervalWindow:
+			// If interval window in a KV, this may be a mapping function.
+			// Special case since windows are not normally used directly as FullValues.
+			return coder.NewIntervalWindowCoder(), nil

Review Comment:
   Even shorter: We do need this "block", it should live in the "outer" switch, not this inner switch.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1324376805

   
   This is a very good start!
   
   > A note about testing: I was unable to write a batch pipeline that resulted in a window mapping operation.
   > 
   > [Here](https://github.com/camphillips22/beam/compare/cam/mapwindows...camphillips22:beam:mapwindows-manual-tests?expand=1) is a link to the tests I wrote which did not result in a map_windows operation. It also contains the changes I made to the streaming wordcap example to make it happen on dataflow. I have not written a pipeline yet that works on Flink and uses map windows, but I wanted to go ahead get this change out for review.
   
   I looked into the Dataflow's graph analyzer and it doesn't use map windows in batch, so that's not amazing for batch testing. We should at least manually validate in streaming mode. I can do this if you like.
   
   We should still have unit tests for the MapWindows Node in the exec package. We should test to the spec (with the KV<nonce, Window> inputs and outputs), per the proto. eg.That we preserve the "nonce", and similar. https://github.com/apache/beam/blob/master/model/pipeline/src/main/proto/org/apache/beam/model/pipeline/v1/beam_runner_api.proto#L285 
   
   I've got a few more specific comments in a moment or two.
   
   
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030725742


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -248,7 +248,9 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 		case urnIntervalWindow:
 			// If interval window in a KV, this may be a mapping function.
 			// Special case since windows are not normally used directly as FullValues.
-			return coder.NewIntervalWindowCoder(), nil
+			value := coder.NewIntervalWindowCoder()
+			t := typex.New(root, key.T, value.T)
+			return &coder.Coder{Kind: kind, T: t, Components: []*coder.Coder{key, value}}, nil

Review Comment:
   Ah, so there is already a case in the switch for that [here](https://github.com/apache/beam/blob/master/sdks/go/pkg/beam/core/runtime/graphx/coder.go#L388-L393), and it talks about special handling for tests. I was hesitant to do anything with that before review. It also looks like there's other behavior that might depend on that being a `Window` coder instead of this new kind. 



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1029897281


##########
sdks/go/pkg/beam/core/graph/coder/coder.go:
##########
@@ -177,6 +177,7 @@ const (
 	Iterable           Kind = "I"
 	KV                 Kind = "KV"
 	LP                 Kind = "LP" // Explicitly length prefixed, likely at the runner's direction.
+	IWCValue           Kind = "IWCvalue"

Review Comment:
   Note how nothing else in this `coder` package calls out it's coderness in their short names. I'd just call this `IntervalWindow` with IW as the short version.
   
   The 'value' sufixes for WindowedValue and ParamWindowedValue indicate they're wrapping a value themselves, rather than being windows themselves, like the one we're adding here.



##########
sdks/go/pkg/beam/core/graph/coder/coder.go:
##########
@@ -177,6 +177,7 @@ const (
 	Iterable           Kind = "I"
 	KV                 Kind = "KV"
 	LP                 Kind = "LP" // Explicitly length prefixed, likely at the runner's direction.
+	IWCValue           Kind = "IWCvalue"
 
 	Window Kind = "window" // A debug wrapper around a window coder.

Review Comment:
   It bother's me we can't re-use this, but I agree we shouldn't mix these up just now. It's an internal implementation detail at least, so it can be cleaned up at some future point.



##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 				t := typex.New(root, append([]typex.FullType{key.T}, coder.Types(values)...)...)
 				return &coder.Coder{Kind: kind, T: t, Components: append([]*coder.Coder{key}, values...)}, nil
 			}
+		case urnIntervalWindow:
+			// If interval window in a KV, this may be a mapping function.
+			// Special case since windows are not normally used directly as FullValues.
+			return coder.NewIntervalWindowCoder(), nil

Review Comment:
   Per the proto "map_windows" spec, the input/output is "KV<[]byte,Window>" but as written here, this just returns "window".  It'd be better if we follow the normal structure, generalizing to handle interval windows better instead of special casing here.



##########
sdks/go/pkg/beam/core/runtime/exec/window.go:
##########
@@ -97,6 +98,55 @@ func (w *WindowInto) String() string {
 	return fmt.Sprintf("WindowInto[%v]. Out:%v", w.Fn, w.Out.ID())
 }
 
+type MapWindows struct {
+	UID UnitID
+	Fn  WindowMapper
+	Out Node
+}
+
+func (m *MapWindows) ID() UnitID {
+	return m.UID
+}
+
+func (m *MapWindows) Up(_ context.Context) error {
+	return nil
+}
+
+func (m *MapWindows) StartBundle(ctx context.Context, id string, data DataContext) error {
+	return m.Out.StartBundle(ctx, id, data)
+}
+
+func (m *MapWindows) ProcessElement(ctx context.Context, elm *FullValue, values ...ReStream) error {
+	w, ok := elm.Elm.(window.IntervalWindow)
+	if !ok {
+		return errors.Errorf("not an IntervalWindow, got %T", elm.Elm)
+	}
+	newW, err := m.Fn.MapWindow(w)
+	if err != nil {
+		return err
+	}
+	out := &FullValue{
+		Elm:       elm.Elm,

Review Comment:
   As written here, it looks like we assume the output nonce is the same as the input window, which may not be the case in all runners. As the coder is written, and picked for a KV, it looks like the mapped element never gets coded.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030697539


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 				t := typex.New(root, append([]typex.FullType{key.T}, coder.Types(values)...)...)
 				return &coder.Coder{Kind: kind, T: t, Components: append([]*coder.Coder{key}, values...)}, nil
 			}
+		case urnIntervalWindow:
+			// If interval window in a KV, this may be a mapping function.
+			// Special case since windows are not normally used directly as FullValues.
+			return coder.NewIntervalWindowCoder(), nil

Review Comment:
   The recursive lookups (to b.Coder) for the key and value types should be able to handle getting the nonce (AKA []byte), and the Interval Window coder.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck merged pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck merged PR #24307:
URL: https://github.com/apache/beam/pull/24307


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030741894


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -248,7 +248,9 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 		case urnIntervalWindow:
 			// If interval window in a KV, this may be a mapping function.
 			// Special case since windows are not normally used directly as FullValues.
-			return coder.NewIntervalWindowCoder(), nil
+			value := coder.NewIntervalWindowCoder()
+			t := typex.New(root, key.T, value.T)
+			return &coder.Coder{Kind: kind, T: t, Components: []*coder.Coder{key, value}}, nil

Review Comment:
   As the person who put that in, I'd rather inconvenience the tests than the production code. Please feel free to change them up. (sorry for the churn).
   That was before I was aware that there were uses for runner sending coders around like that, and we just wanted to validate the coders per the standard_coders.yaml tests.
   
   Those uses are handled here usually: https://github.com/camphillips22/beam/tree/c83b42f7547a5fd57e2f9a90d7b613c512eb088c/sdks/go/test/regression/coders/fromyaml



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030616603


##########
sdks/go/pkg/beam/core/graph/coder/coder.go:
##########
@@ -177,6 +177,7 @@ const (
 	Iterable           Kind = "I"
 	KV                 Kind = "KV"
 	LP                 Kind = "LP" // Explicitly length prefixed, likely at the runner's direction.
+	IWCValue           Kind = "IWCvalue"

Review Comment:
   I had named it `IntervalWindow` to start, but there's a conflicting `IntervalWindow` value define [here](https://github.com/camphillips22/beam/blob/c83b42f7547a5fd57e2f9a90d7b613c512eb088c/sdks/go/pkg/beam/core/graph/coder/windows.go#L25-L25). Maybe just `IW` for const name with a comment that explicitly calls out that it refers to an `IntervalWindow`? Any other suggestions?
   
   I can definitely change the value to "IW" though.



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] camphillips22 commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
camphillips22 commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331232215

   @lostluck Anything left to do here other than resolve conflicts?


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] github-actions[bot] commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1323021294

   Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030955511


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -379,18 +379,14 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 			return nil, err
 		}
 		return coder.NewN(elm), nil
-
-		// Special handling for window coders so they can be treated as
-		// a general coder. Generally window coders are not used outside of
-		// specific contexts, but this enables improved testing.
-		// Window types are not permitted to be fulltypes, so
-		// we use assignably equivalent anonymous struct types.
 	case urnIntervalWindow:

Review Comment:
   Now that we have a coder* for it, we should ensure that the coder can be round trip serialized. Please add an Unmarshal(Marshal(*coder*)) round trip test (see graphx/coder_test.go)



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] codecov[bot] commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1324379093

   # [Codecov](https://codecov.io/gh/apache/beam/pull/24307?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#24307](https://codecov.io/gh/apache/beam/pull/24307?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f95df24) into [master](https://codecov.io/gh/apache/beam/commit/79b8c154b58ead00dc4b3347d7e691e3dba52010?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (79b8c15) will **decrease** coverage by `0.03%`.
   > The diff coverage is `22.72%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #24307      +/-   ##
   ==========================================
   - Coverage   73.45%   73.41%   -0.04%     
   ==========================================
     Files         714      714              
     Lines       96506    96571      +65     
   ==========================================
   + Hits        70887    70898      +11     
   - Misses      24297    24348      +51     
   - Partials     1322     1325       +3     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | go | `51.38% <22.72%> (-0.08%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/beam/pull/24307?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [sdks/go/pkg/beam/core/graph/coder/coder.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL2dyYXBoL2NvZGVyL2NvZGVyLmdv) | `84.68% <0.00%> (-0.82%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/exec/translate.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy90cmFuc2xhdGUuZ28=) | `12.52% <0.00%> (-0.21%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/exec/window.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy93aW5kb3cuZ28=) | `31.25% <0.00%> (-17.77%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/graphx/coder.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZ3JhcGh4L2NvZGVyLmdv) | `51.44% <0.00%> (-0.47%)` | :arrow_down: |
   | [sdks/go/pkg/beam/core/runtime/graphx/translate.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZ3JhcGh4L3RyYW5zbGF0ZS5nbw==) | `38.43% <ø> (ø)` | |
   | [sdks/go/pkg/beam/core/runtime/exec/coder.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9jb2Rlci5nbw==) | `58.03% <70.00%> (+0.28%)` | :arrow_up: |
   | [sdks/go/pkg/beam/core/runtime/exec/fullvalue.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL3J1bnRpbWUvZXhlYy9mdWxsdmFsdWUuZ28=) | `77.24% <100.00%> (ø)` | |
   | [sdks/go/pkg/beam/core/metrics/dumper.go](https://codecov.io/gh/apache/beam/pull/24307/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2Rrcy9nby9wa2cvYmVhbS9jb3JlL21ldHJpY3MvZHVtcGVyLmdv) | `49.20% <0.00%> (-4.77%)` | :arrow_down: |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on a diff in pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on code in PR #24307:
URL: https://github.com/apache/beam/pull/24307#discussion_r1030707840


##########
sdks/go/pkg/beam/core/runtime/graphx/coder.go:
##########
@@ -245,6 +245,10 @@ func (b *CoderUnmarshaller) makeCoder(id string, c *pipepb.Coder) (*coder.Coder,
 				t := typex.New(root, append([]typex.FullType{key.T}, coder.Types(values)...)...)
 				return &coder.Coder{Kind: kind, T: t, Components: append([]*coder.Coder{key}, values...)}, nil
 			}
+		case urnIntervalWindow:
+			// If interval window in a KV, this may be a mapping function.
+			// Special case since windows are not normally used directly as FullValues.
+			return coder.NewIntervalWindowCoder(), nil

Review Comment:
   Basically, the Iterable/StateBackedIterable cases need special handling because technically CoGBK results are KV<k, Iter<V>> (or even KV<k, Iter<V1>, ... , Iter<Vn>>), but we avoid degenerate KVs with more than 2 components in the Go SDK, by having a dedicated CoGBK "kind" for coders.  
   
   This lets us treat the data ingestion properly datasource.go, and be able to do the special handling to convert to the pull iterator functions `func(*V) bool` (and with any luck, once Go settles on an Iterator convention, support that style as well.)
   
   Basically, if we had "real" KV support as a single KV element instead of splitting K and V parameters, we wouldn't need to go this far, but we're stuck with supporting it in the current API.
   
   But this is a long way of saying, we don't need to special case things at this layer since there's nothing "tricky" about the element type technically, we just put Windows into their own box to avoid accidental mistakes (or generally nonsensical user mistakes like "processing" the internal windows metadata)
   



-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331240506

   I think just resolving the conflicts? I'll take another look after i finish lunch.


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [beam] lostluck commented on pull request #24307: Add map_windows support to Go SDK

Posted by GitBox <gi...@apache.org>.
lostluck commented on PR #24307:
URL: https://github.com/apache/beam/pull/24307#issuecomment-1331359445

   Run Go Flink ValidatesRunner


-- 
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: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org