Typed Search Attributes #1368

cretz · 2024-01-29T18:58:36Z

What was changed

Implementation of typed search attributes. Specifically:

Added TypedSearchAttributes to client.StartWorkflowOptions and deprecated SearchAttributes
Added several new things to temporal package including:
- SearchAttributes collection with methods and constructor
- SearchAttributeKey and SearchAttributeUpdate interfaces
- SearchAttributeKeyX types and constructors for typed keys
Added TypedSearchAttributes to client.ScheduleWorkflowAction and 💥removed SearchAttributes and added UntypedSearchAttributes (needed until Eagerly validate and put proper metadata on search attributes for on schedule workflow action temporal#4787 is fixed)
Added TypedSearchAttributes to client.ScheduleOptions and deprecated SearchAttributes
Added GetTypedSearchAttributes to workflow and deprecated WorkflowInfo.SearchAttributes
Added UpsertTypedSearchAttributes to workflow and deprecated UpsertSearchAttributes
Tests

Much of this work was done by @Quinn-With-Two-Ns

💥 BREAKING CHANGE

Users that used SearchAttributes on client.ScheduleWorkflowAction will now have a compile-time failure (not to be confused with search attributes on the schedule itself). There was no good way to have both untyped and typed coexist for create, describe, and update of a workflow action on a schedule. The release notes should document this break intentionally. This is the same decision we reached in Python.

Checklist

Closes [Feature Request] Typed search attributes #1218

NOTE: Currently there are two dev-server bugs (lack of TemporalScheduledX search attrs in workflow and a case where a SA name is appearing as Text01) that I'm hoping have since been fixed and are awaiting CLI release. That's why CI is failing, but can still review.

Quinn-With-Two-Ns · 2024-01-30T00:06:13Z

workflow/workflow.go

 // [Visibility]: https://docs.temporal.io/visibility
 func UpsertSearchAttributes(ctx Context, attributes map[string]interface{}) error {
 	return internal.UpsertSearchAttributes(ctx, attributes)
 }

+// UpsertTypedSearchAttributes is used to add, update, or remove workflow search attributes. The search attributes can
+// be used in query of List/Scan/Count workflow APIs. The key and value type must be registered on temporal server side;
+// The value has to deterministic when replay; The value has to be Json serializable.


The value has to deterministic when replay

It does?

I just took this doc from UpsertSearchAttributes. But I just confirmed that we only check event type for upsert. So I will remove this statement from both Godocs on next commit (still going to wait for CLI publish to see if some CLI errors are solved).

Quinn-With-Two-Ns · 2024-01-30T16:34:17Z

What happens if I mix and match upserting typed and untyped search attributes and what do the typed and untyped getters return ?

Sushisource · 2024-01-30T18:14:03Z

internal/internal_search_attributes.go

+	SearchAttributeKeyKeywordList struct {
+		baseSearchAttributeKey
+	}
+)


Can we do a SearchAttributeKeyRawPayload? As I found while doing Omes stuff, being able to pass though payloads is very convenient in some cases. We can add a docstring to the effect of "this is for advanced use cases only"

We can, though I'm not as big of a fan of us adding tools for ourselves only to save a switch or similar. @Quinn-With-Two-Ns - thoughts? (also worth discussing whether it's this PR or another)

I don't think it's only useful for us - it's useful when doing something like copying data from one workflow to another, or enabling things like the use case we were talking about where someone might scrape a workflow's history and apply things in it to the current workflow.

That is a general problem with the inability to work with raw values in Go. The goal here was to not be able to provide the incorrect type even if you wanted to. But I don't mind the "raw" key if @Quinn-With-Two-Ns also agrees. Also open to other ideas that doesn't put it on par with all other properly-typed keys.

Hm to copy from one workflow to another can't a user do SearchAttributes.Copy() ? If the only use is out Omes DSL I wouldn't add "raw" keys. If we did want it it should be in all languages not just Go, so maybe we should move the discussion out of this PR?

Well they may not necessarily be other SAs is the problem. It could be input to the workflow, from a signal, etc, which you're accepting as a Payload

@Sushisource can we open a features issue to discuss this? I think it's not included in Chads original proposal for TS attributes temporalio/proposals#75 and if we did do it I'd want it in all langs that make sense not just Go.

cretz · 2024-01-30T18:47:26Z

What happens if I mix and match upserting typed and untyped search attributes

Can you clarify the scenario a bit better? Do you mean for something like workflow start? Or workflow use?

and what do the typed and untyped getters return ?

Untyped getter returns what it always had, no behavior change there. Typed returns only type-present search attributes which is supposed to be all of them.

Quinn-With-Two-Ns · 2024-01-30T22:52:21Z

Can you clarify the scenario a bit better? Do you mean for something like workflow start? Or workflow use?

In a workflow someone calling a mix of the typed and untyped upsert

Untyped getter returns what it always had, no behavior change there. Typed returns only type-present search attributes which is supposed to be all of them.

So if a user uses the typed API to upsert will the result not be present in the untyped getter?

cretz · 2024-01-30T23:15:51Z

In a workflow someone calling a mix of the typed and untyped upsert

Each would result in independent commands. Untyped behaves as it always has, typed behaves as expected. I'm not sure we need to solve for advanced cases of mixing these in the same task.

So if a user uses the typed API to upsert will the result not be present in the untyped getter?

I just copied your previous code which seems to delegate both to the environment's UpsertSearchAttributes which updates the one on info. And the typed getter converts live on each get call. So both update each other's it seems.

Quinn-With-Two-Ns · 2024-01-30T23:31:33Z

test/integration_test.go

@@ -1221,6 +1221,15 @@ func (ts *IntegrationTestSuite) TestWorkflowWithParallelMutableSideEffects() {
 	ts.NoError(ts.executeWorkflow("test-wf-parallel-mutable-side-effects", ts.workflows.WorkflowWithParallelMutableSideEffects, nil))
 }

+func (ts *IntegrationTestSuite) TestWorkflowTypedSearchAttributes() {


Can we have a test that confirms TemporalChangeVersion is set properly and accessible with typed search attributes?

👍 Will add (next commit, was waiting for CLI to release dev server with temporalio/temporal#5124, but looks like that's not even released server side)

Added test. I am setting the index value type to KeywordList when converting to typed if not already set because I felt this was the safer route vs changing how the search attribute is set.

…tes (#2167) temporalio/sdk-go#1368

Typed search attributes

599e655

cretz force-pushed the typed-search-attributes branch 3 times, most recently from 8fb7264 to 298a4a9 Compare January 29, 2024 21:19

More search attribute work

6679d9c

cretz force-pushed the typed-search-attributes branch from 298a4a9 to 6679d9c Compare January 29, 2024 22:10

cretz marked this pull request as ready for review January 29, 2024 22:46

cretz requested a review from a team as a code owner January 29, 2024 22:46

Quinn-With-Two-Ns reviewed Jan 30, 2024

View reviewed changes

Sushisource reviewed Jan 30, 2024

View reviewed changes

Quinn-With-Two-Ns reviewed Jan 30, 2024

View reviewed changes

Typed search attribute fixes

8a917bc

cretz force-pushed the typed-search-attributes branch from 16d5d3b to 8a917bc Compare January 31, 2024 22:25

Quinn-With-Two-Ns approved these changes Feb 1, 2024

View reviewed changes

Merge branch 'master' into typed-search-attributes

3a43319

cretz merged commit 57b6679 into temporalio:master Feb 1, 2024
12 checks passed

cretz deleted the typed-search-attributes branch February 1, 2024 18:46

serprex mentioned this pull request Oct 21, 2024

temporal: replace deprecated SearchAttributes with TypedSearchAttributes PeerDB-io/peerdb#2167

Merged

serprex added a commit to PeerDB-io/peerdb that referenced this pull request Oct 21, 2024

temporal: replace deprecated SearchAttributes with TypedSearchAttribu…

895c643

…tes (#2167) temporalio/sdk-go#1368

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typed Search Attributes #1368

Typed Search Attributes #1368

cretz commented Jan 29, 2024 •

edited

Loading

Quinn-With-Two-Ns Jan 30, 2024

cretz Jan 30, 2024

cretz Jan 31, 2024

Quinn-With-Two-Ns commented Jan 30, 2024

Sushisource Jan 30, 2024

cretz Jan 30, 2024

Sushisource Jan 30, 2024

cretz Jan 30, 2024

Quinn-With-Two-Ns Jan 30, 2024

Sushisource Jan 31, 2024

Quinn-With-Two-Ns Feb 1, 2024

cretz commented Jan 30, 2024

Quinn-With-Two-Ns commented Jan 30, 2024

cretz commented Jan 30, 2024 •

edited

Loading

Quinn-With-Two-Ns Jan 30, 2024

cretz Jan 31, 2024 •

edited

Loading

cretz Jan 31, 2024

Typed Search Attributes #1368

Typed Search Attributes #1368

Conversation

cretz commented Jan 29, 2024 • edited Loading

What was changed

💥 BREAKING CHANGE

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quinn-With-Two-Ns commented Jan 30, 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

Choose a reason for hiding this comment

cretz commented Jan 30, 2024

Quinn-With-Two-Ns commented Jan 30, 2024

cretz commented Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

cretz Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz commented Jan 29, 2024 •

edited

Loading

cretz commented Jan 30, 2024 •

edited

Loading

cretz Jan 31, 2024 •

edited

Loading