Add OR statement to match V1 and V2 Scheduler BusinessIds#10248
Merged
Conversation
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
1b0fed5 to
07767a4
Compare
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
07767a4 to
2294a95
Compare
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
2294a95 to
e1d0bb7
Compare
chaptersix
reviewed
May 14, 2026
| // This block runs after the STARTS_WITH → LIKE conversion above so the V1 clone inherits the | ||
| // already-converted operator and escape settings. | ||
| // TODO: once V1 schedules are fully migrated to CHASM, remove this OR/AND block and only use V2 (unprefixed) values. | ||
| if saColNameExpr.alias == sadefs.ScheduleID && c.archetypeID == chasm.SchedulerArchetypeID { |
Contributor
There was a problem hiding this comment.
🤖 Missing fieldName guard — the generic converter (converter.go:399) and ES legacy converter (converter_legacy.go:319) both check colName.FieldName == sadefs.WorkflowID here, but this condition only checks the alias. If a user defines a custom ScheduleId search attribute (which would map to a different field name), this block would still fire and incorrectly inject the V1 prefix OR/AND logic.
Suggested change
| if saColNameExpr.alias == sadefs.ScheduleID && c.archetypeID == chasm.SchedulerArchetypeID { | |
| if saColNameExpr.alias == sadefs.ScheduleID && saColNameExpr.fieldName == sadefs.WorkflowID && c.archetypeID == chasm.SchedulerArchetypeID { |
Contributor
There was a problem hiding this comment.
I'm not super familiar with this portion of the codebase.
Contributor
Author
There was a problem hiding this comment.
ah this is correct, just need to verify the field name, thanks
chaptersix
reviewed
May 14, 2026
| }, 15*time.Second, 1*time.Second) | ||
| } | ||
|
|
||
| func testListSchedulesFilterByScheduleID(t *testing.T, newContext contextFactory) { |
Contributor
There was a problem hiding this comment.
this set test only checks v1 xor v2. could you add one that has results for both?
chaptersix
reviewed
May 14, 2026
|
|
||
| { | ||
| name: "CHASM path IN generates OR of prefixed and unprefixed", | ||
| in: "ScheduleId IN ('foo', 'bar')", |
lina-temporal
approved these changes
May 14, 2026
Contributor
lina-temporal
left a comment
lina-temporal
left a comment
There was a problem hiding this comment.
few small comments, otherwise LGTM; would appreciate a stamp from visi/foundations as well, though.
| }) | ||
|
|
||
| // Not equal: ScheduleId != 'sid1' should return sid2 but not sid1. | ||
| t.Run("NotEqual", func(t *testing.T) { |
Contributor
There was a problem hiding this comment.
Can you also add the Len assertions on the cases with 2 schedules returned? This will guard against something in the handler returning duplicate results.
Comment on lines
+491
to
+493
| if comparisonExpr.Operator == sqlparser.NotEqualStr || | ||
| comparisonExpr.Operator == sqlparser.NotInStr || | ||
| comparisonExpr.Operator == sqlparser.NotStartsWithStr { |
Contributor
There was a problem hiding this comment.
(minor) Can you reuse isNegativeScheduleIDOperator here?
Comment on lines
+409
to
+411
| isNegative := expr.Operator == sqlparser.NotEqualStr || | ||
| expr.Operator == sqlparser.NotInStr || | ||
| expr.Operator == sqlparser.NotLikeStr |
Contributor
There was a problem hiding this comment.
(minor) Ditto for reusing isNegativeScheduleIDOperator here
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
2 times, most recently
from
0922434 to
3e13a0f
Compare
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
3e13a0f to
254bea8
Compare
rodrigozhou
previously requested changes
May 14, 2026
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
a4ad61e to
6c25432
Compare
awln-temporal
force-pushed
the
awln/scheduler-visibility-fix
branch
from
6c25432 to
7512038
Compare
awln-temporal
dismissed
rodrigozhou’s stale review
review is complete, pending discussion has been done offline
rodrigozhou
approved these changes
May 15, 2026
yycptt
pushed a commit
that referenced
this pull request
May 18, 2026
…10316) ## What changed? Add a Scheduler specific query converter to handle ScheduleId search attribute alias to underlying WorkflowId system search attribute field. If chasm is enabled, the query converter will match against both V1 and V2 Scheduler workflowId formats. V1 prefixes the workflowId with temporal-sys-scheduler, while V2 doesn't, so to handle either case, we need to add a OR/AND statement to match both V1 and V2 Scheduler BusinessId. OR will be used if the operator's boolean is positive, AND if the operator's boolean is negative. ## Why? Unable to retrieve V2 Schedules that query using ScheduleId as a search attribute. ## How did you test it? - [X] built - [X] run locally and tested manually - [X] covered by existing tests - [X] added new unit test(s) - [X] added new functional test(s)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed?
Add a Scheduler specific query converter to handle
ScheduleIdsearch attribute alias to underlyingWorkflowIdsystem search attribute field. If chasm is enabled, the query converter will match against both V1 and V2 Scheduler workflowId formats. V1 prefixes the workflowId withtemporal-sys-scheduler, while V2 doesn't, so to handle either case, we need to add a OR/AND statement to match both V1 and V2 Scheduler BusinessId. OR will be used if the operator's boolean is positive, AND if the operator's boolean is negative.Why?
Unable to retrieve V2 Schedules that query using
ScheduleIdas a search attribute.How did you test it?