Please change the var names.

why we need to take in both?

NamespaceID is what we store in persistence, and NamespaceName is needed for dynamic config and custom search attributes mapper.

interesting that matching needs visibility manager as well.

The scheduler has some constraints in their query, so it needs the visibility manager to build the query converter IIRC.

plz also comment that those methods should not be used by chasm library developer. and also for RegistrableComponent.SearchAttributesMapper()

we probably need to create a separate interface for developer facing registry later.

hmm I think now that we are using proto.Message we no longer have guarantee that there's no error upon encoding in task executor, but guess that should never happen.

Something for later, since the currentMemo/SA here are always consistent with CHASM tree state, in visibility task executor we actually don't have to invoke SA/Memo provider again, we can just serialize & buffer the value here (and now we can also return error upon closeTransaction if it can't be encoded), and use it in the vis task executor.

Now that persistence can depend on chasm package, for those new struct definitions, is it possible to reuse the corresponding definitions in chasm package and thus avoiding the conversion between the different struct definition of essentially the same thing in chasm_engine.go?

we can definitely reuse the return struct between the vis manager and ListExecutions in chasm_engine.go, i'm currently keeping separate request types since there is a difference when calling vis manager (archetypeType in engine.go vs archetypeID in vis manager)

is this intentional?

no, thanks for catching this

hmm why not just check if __chasm__ is a key in the combinedMemo?

if we are worrying about potential conflict (though I feel that chance is very low...), I'd probably just check if TemporalNamespaceDivision string is a number or not (just the first char should be enough), since we are going to use this method to query across archetypes, e.g. in namespace migration case, I just want all visibility records.

Yes, I'm concerned about potential conflict (although very low).
I'm ok with checking if TemporalNamespaceDivision is a number.

moved to just checking if number. We can check the first char if we think that optimization is needed, just thought checking the entire string might be more clear.

Please change the var names.

I think we can probably use generics here and have just a single Get method.
It would require defining another TypedSearchAttribute[T] interface with a single typeMarker(T) method and making sure that SearchAttributeBool and friend implement that interface.

yea, i like this approach, making a change for this

You should document this behavior. I'm also on the fence on whether we should error out here or panic or just return false.

Current behavior silently returns not found if any type checks fail. Added documentation for the runtime check before casting, returning a zero val and false if unable to cast successfully.

For the first cast/check to a VisibilityValue, the case this fails is when a Developer doesn't register search attributes and has duplicate aliases.

Second cast from VisibilityValue to underlying value would fail if search attribute author (me) did incorrect mappings between VisibilityValue and its type.

Unit tests are able to catch cases for the Second cast. Ideally, functional tests should be able to cover the first case? I'm not sure if there's any benefit to panicking, but I can see if we threw errors, we could also get different handling of cases, etc.

Let me know your thoughts.

Do you need this?

I think it makes the mapping function more concise? Otherwise we need to switch on each visibility type to its underlying primitive right

also makes it possible to specify the correct type in the method instead of needing to return any

I think lines L465-474 are unnecessary. You could simply do result, ok := visibilityValue.(T). If ok is true, then it good, otherwise, it's not.

yea good point lol

original suggestion was supposed to combine type maps, but got overridden while going through merge conflicts, changed to

combinedTypeMap := make(map[string]enumspb.IndexedValueType)
maps.Copy(combinedTypeMap, vi.searchAttributesTypeMap.Custom())
maps.Copy(combinedTypeMap, vi.chasmMapper.SATypeMap())
finalTypeMap := searchattribute.NewNameTypeMap(combinedTypeMap)

fyi keeping the combined type map initialization on separate line since there were potential panics if either custom or chasm search attribute type map is nil.

I think we should still return an error. Why are you changing it?

Original thread: #8558 (comment).
The original context was about deregistering search attributes while the task is getting processed, which would lead to task failure.

After looking at this a bit more, any upsert SA task execution would still fail when generating the ES Doc and attempting to decode the value, if the search attribute has been deregistered.

In the SQL implementation though, since these fields are constant and always attached to the NameTypeMap/ClusterMetadata, Upserting these fields would succeed, so there is a discrepancy there.

I think we should not be throwing an error here though. Assuming we want parity with Upserting search attributes for workflows, once we have parity between ES and SQL for preallocated columns, we should not throw an error if a search attribute gets deregistered when executing the CHASM vis task, and just do a best effort upsert call. Otherwise, this task would get retried and sent to DLQ if I understand correctly.