materialize icon indicating copy to clipboard operation
materialize copied to clipboard

Fix visitation in `HirRelationExpr::contains_temporal`

Open ggevay opened this issue 1 year ago • 0 comments

The problem was that contains_temporal was just calling visit_children, which doesn't do a visitation in the entire subtree, but merely visits the scalar expressions in the top-level relation expr node. This PR changes contains_temporal to do a visitation of all relation expr nodes, and for each of them do what we were doing before for just the top-level node.

The issue description mentions that we should be able to find temporal expressions also in referenced view definitions. This already works: peek_stage_validate calls validate_timeline_context, which calls get_timeline_contexts, which transitively visits view definitions and calls MirRelationExpr::contains_temporal() on their optimized expressions.

Motivation

  • This PR partially fixes a recognized bug: https://github.com/MaterializeInc/materialize/issues/25339
    • It doesn't fix it for could_run_expensive_function.

Tips for reviewer

Checklist

  • [ ] This PR has adequate test coverage / QA involvement has been duly considered.
  • [ ] This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • [ ] If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • [ ] This PR includes the following user-facing behavior changes:

ggevay avatar Feb 17 '24 12:02 ggevay