[Visibility] Fix legacy visibility query converter - #11910
Merged
Conversation
rodrigozhou
added
reliability-2026
rodrigozhou
commented
Sep 2, 2026
Contributor
Author
There was a problem hiding this comment.
The function ResolveSearchAttributeAlias was used only in the legacy query converter. In this PR, I copied the function from QueryConverter.resolveSearchAttributeAlias and replaced the one here. I also replaced the unit tests accordingly and added further cases that were not covered.
rodrigozhou
commented
Sep 2, 2026
| return colName, nil | ||
| } | ||
|
|
||
| func (c *QueryConverter[ExprT]) resolveSearchAttributeAlias( |
Contributor
Author
There was a problem hiding this comment.
This function was moved to resolve.go file
rodrigozhou
force-pushed
the
rodrigozhou/vis-qc-legacy
branch
3 times, most recently
from
September 3, 2026 02:22
1997d8b to
96d01e3
Compare
| // Fourth, check for special aliases or adding/removing the `Temporal` prefix. | ||
| if strings.TrimPrefix(alias, sadefs.ReservedPrefix) == sadefs.ScheduleID { | ||
| fieldName = sadefs.WorkflowID | ||
| } else if archetypeID == chasm.SchedulerArchetypeID && alias == "TemporalSystemExecutionStatus" { |
Contributor
There was a problem hiding this comment.
so this was missing in the legacy path? What else am I missing?
Contributor
Author
There was a problem hiding this comment.
Yes, I think that's the only thing missing.
| e.Operator, | ||
| ) | ||
| } | ||
| if value, ok := e.Expr.(*sqlparser.SQLVal); !ok || value.Type == sqlparser.StrVal { |
Contributor
There was a problem hiding this comment.
can I do something like -true? Is it necessarily either number value or string val?
Contributor
Author
There was a problem hiding this comment.
It's parsed by sqlparser, but bool values are not parsed as sqlparser.StrVal, it's sqlparser.BoolVal. This code would reject it.
awln-temporal
approved these changes
Sep 3, 2026
rodrigozhou
force-pushed
the
rodrigozhou/vis-qc-legacy
branch
from
September 4, 2026 00:34
96d01e3 to
96eb204
Compare
rodrigozhou
enabled auto-merge (squash)
September 4, 2026 00:51
| }) | ||
| } | ||
|
|
||
| func TestQueryConverter_ResolveSearchAttributeAlias_WithChasmMapper(t *testing.T) { |
Contributor
There was a problem hiding this comment.
do we have coverage for this?
Contributor
Author
There was a problem hiding this comment.
Yes. The copy from the query converter that I copied from covers this.
davidporter-id-au
approved these changes
Sep 4, 2026
davidporter-id-au
left a comment
davidporter-id-au
left a comment
Contributor
There was a problem hiding this comment.
haven't been able to give it the most thorough review, but broadly looks ok, I realise you're rushing the release cut
5 tasks
simvlad
pushed a commit
that referenced
this pull request
Sep 4, 2026
## What changed? - Fix parsing negative double values. - Consolidated the function to resolve alias (there were two, and not behaving the same way, the legacy was missing some cases). - Added more unit tests to verify the legacy query converter behaves the same as in the new one except for the expected differences. ## Why? Fixes to legacy visibility query converter for SQL and Elasticsearch. ## How did you test it? - [x] built - [ ] run locally and tested manually - [x] covered by existing tests - [x] added new unit test(s) - [ ] added new functional test(s) ## Potential risks (cherry picked from commit 9979699)
simvlad
added a commit
that referenced
this pull request
Sep 4, 2026
## What changed? Backports the 12 merged PRs labeled [release/1.32.0](https://github.com/temporalio/temporal/issues?q=repo%3Atemporalio%2Ftemporal+is%3Apr+label%3Arelease%2F1.32.0) that were not yet on `release/v1.32.x`, applied in `main` merge order. Also fixes mixedbrain test by bumping go version to 1.26.8 in mixedbrain folder (broken by #11928). | # | PR | Author | What / Why | |---|----|--------|------------| | 1 | [#11546](#11546) fix(batcher): scope deterministic request IDs to the batch job ID | @spkane31 | Two batch operations sending the same signal to the same workflow/run ID only delivered one — the request ID was hashed from workflow ID / run ID / signal name, so the second was de-duped in signal logic. Including the batch job ID in the hash keeps separate signals distinct. | | 2 | [#11642](#11642) Fix batch operations targeting paused executions | @fretz12 | A paused execution is still non-terminal, but the old `Running`-only filter silently skipped paused targets. Users issuing a batch terminate/signal/cancel reasonably expect it to apply to them. | | 3 | [#11666](#11666) Fix activity timeout regeneration after unpause | @fretz12 | Timeout tasks that fired while an activity was paused were discarded, but the activity's timer-task status still claimed they existed — preventing recreation after unpause and potentially making the timeout ineffective. Also fixes `ResetActivity` bypassing normal unpause handling, and running activities returning early without clearing paused state. | | 4 | [#11696](#11696) Treat missing Worker Deployment Version as deleted | @rkannan82 | Treats `NotFound` from the Version workflow delete update as success. When a Version workflow is already closed or its history is gone, `NotFound` blocked the Deployment workflow from removing its stale version reference. Fixes #11539. | | 5 | [#11725](#11725) Fix batch activity unpause visibility query scope | @fretz12 | The server replaced the caller's visibility query with the activity-type predicate, which could broaden the batch scope and unpause activities in workflows the caller never selected. | | 6 | [#11806](#11806) Fix activity operator command metric label collision | @fretz12 | WFA and SAA emitted the same Prometheus metric names (`activity_pause`/`unpause`/`reset`/`update_options`) with **different label sets**, causing descriptor registration failures and dropped metrics. Both now use the standard per-activity label schema and emit once per affected activity. **See "Backport adaptation" below.** | | 7 | [#11930](#11930) Prioritize worker versioning workflows | @ShahabT | Worker versioning workflows share the per-namespace worker task queue with other system workflows. Raising their initial workflow-task priority above default keeps versioning operations from queueing behind default-priority work. | | 8 | [#11910](#11910) [Visibility] Fix legacy visibility query converter | @rodrigozhou | Fixes to the legacy visibility query converter for both SQL and Elasticsearch. | | 9 | [#11699](#11699) dynamic partitioning: use probability of AddTask to root partition to estimate overall add task rate | @carlydf | A previous PR changed AddTask load-balancing from uniform random to backlog-aware, invalidating the uniform-distribution assumption behind the add-task-rate estimate. Now estimates from the probability of AddTask hitting the root partition, and guarantees the root retains at least a 1% chance of selection regardless of its backlog. | | 10 | [#11935](#11935) [Visibility] Enable unified query converter by default | @rodrigozhou | Flips the default now that the unified query converter is stable and ready to replace the old one. Depends on #11910 and #11801. | ## How did you test it? - [x] built — `go build ./...` - [x] `make fmt` — no changes - [x] `go vet -tags disable_grpc_modules,test_dep ./tests/...` — clean - [x] `make GOLANGCI_LINT_FIX=false GOLANGCI_LINT_BASE_REV=origin/release/v1.32.x lint-code` — **0 issues** - [x] covered by existing tests — 21 affected packages, **20 pass** --------- Co-authored-by: Sean Kane <spkane31@gmail.com> Co-authored-by: Fred Tzeng <41805201+fretz12@users.noreply.github.com> Co-authored-by: Kannan <rkannan82@users.noreply.github.com> Co-authored-by: Shahab Tajik <shahab@temporal.io> Co-authored-by: Rodrigo Zhou <rodrigo.zhou@temporal.io> Co-authored-by: Carly de Frondeville <carly.defrondeville@temporal.io> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Vladyslav Simonenko <vlad.simonenko@temporal.io>
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
3 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?
Why?
Fixes to legacy visibility query converter for SQL and Elasticsearch.
How did you test it?
Potential risks