Revert "refactor: Change all Visitors to be iterative, child-based"#36905
Merged
Conversation
ggevay
approved these changes
Jun 4, 2026
antiguru
added a commit
that referenced
this pull request
Jun 8, 2026
### Motivation The iterative, child-based `Visitor` refactor (#36852) was reverted in #36905 because it introduced a regression: SQL-332, where `mz_now()` used as a direct argument to a value window function (e.g. `first_value`, `last_value`) or an aggregate window function (e.g. `max`) was no longer recognized as temporal, so the query failed to receive a timestamp near the current time. For example, `SELECT first_value(mz_now()) OVER () < '3000-01-01'` returned `false` instead of `true`. This PR re-lands the refactor with that regression fixed, so it doesn't ship again. ### Description This PR consists of two commits: 1. **Reapply "refactor: Change all `Visitor`s to be iterative, child-based" (#36852)** — reverts the revert (#36905), restoring the iterative traversal. 2. **Fix temporal detection (SQL-332)** — the root cause of the regression. The iterative `Visit` traversals walk expressions via the new `VisitChildren::children()` / `children_mut()` iterators. For `ValueWindowExpr` and `AggregateWindowExpr`, the new `children()` impls descended *into* the argument expression and yielded *its* children, rather than yielding the argument expression itself — which is what the canonical `visit_children` does, and what the impl-level doc comments promise (*"Yields `args`; does not descend into it"*). `contains_temporal()` runs over this traversal (via `visit_post`). When the argument is a leaf like `mz_now()` (a `CallUnmaterializable` with no children), descending skipped it entirely, so the `mz_now()` node was never visited and the query was planned as a constant. The fix yields the argument expression itself via `std::iter::once(...)`, matching `visit_children`. ### Verification Green CI. Added regression tests in `test/sqllogictest/temporal.slt` covering `mz_now()` as the direct argument of `first_value`, `last_value`, and `max` window functions — each asserts the value is `< '3000-01-01'`, confirming the query receives a current timestamp rather than `u64::MAX`. https://claude.ai/code/session_017Xe6H3p7ZETe4LW5bqPiWZ --------- Co-authored-by: Claude <noreply@anthropic.com>
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
2 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.
Reverts #36852
Causes SQL-332.