refactor: rename PromQL-specific QueryExpr nodes (and Merge/Distinct/WindowFunc/EvalTime) for clarity - #226
Merged
Merged
Conversation
…ames Renames several QueryExpr variants (and two operator-kind types) whose names don't signal what they are or which language they're for: - Scalar, EvalTime, VectorFromScalar, ScalarFromVector, Relabel, InfoJoin, Sample, Subquery -> Promql*-prefixed or renamed, since nothing in the old names marked them PromQL-only - Distinct -> Dedup (collided with AggIntent::Cardinality's COUNT(DISTINCT ...) in conversation/review) - Merge -> Concat, plus QueryExprError::EmptyMerge -> EmptyConcat (collided conceptually with SQL JOIN / merge-join) - WindowFunc -> SQLWindowFunc (one character from the reserved, unimplemented streaming Window node) - Arith -> Arithmetic (QueryExpr::Arith and BinaryOpKind::Arith, the same concept in two places) - ArithOp -> ArithmeticOpKind, CompareOp -> CompareOpKind, matching the codebase's existing Kind-suffix convention for these enums - EvalTime -> QueryTimestamp specifically (not TimeNow, which reads as a different meaning than what EvalTime denoted) Compare is left unchanged. Pure rename; no behavior change. Closes #183 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
changed the title
This was referenced Aug 22, 2026
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
…og (#225) Rebased onto main now that #227 (the catalog crate + generalized ClickHouse-builtin mechanism) and #226 (the QueryExpr rename) have landed there — this PR's own content is unchanged, just reapplied cleanly on top of current main instead of the now-deleted feat/sql-function-catalog-225 branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
Rebased onto main now that #226 (the QueryExpr rename) and #227 (the SQL function catalog) have landed there. This PR's own design and content are unchanged; adapted the new cse.rs module's exhaustive QueryExpr match to the current post-rename variant names (Merge -> Concat, Distinct -> Dedup, Scalar -> PromqlScalar, EvalTime -> QueryTimestamp, WindowFunc -> SQLWindowFunc, Arith -> Arithmetic, VectorFromScalar/ScalarFromVector/Relabel/InfoJoin/Sample/Subquery -> their Promql*-prefixed names, ArithOp/CompareOp -> ArithmeticOpKind/ CompareOpKind) and reapplied mod.rs's doc/pub-use additions around the renamed expr_ir re-export. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
#235) Rebased onto main now that #226 (the QueryExpr rename) and #227 (the SQL function catalog) have landed there. This PR's own design and content are unchanged; adapted the new cse.rs module's exhaustive QueryExpr match to the current post-rename variant names (Merge -> Concat, Distinct -> Dedup, Scalar -> PromqlScalar, EvalTime -> QueryTimestamp, WindowFunc -> SQLWindowFunc, Arith -> Arithmetic, VectorFromScalar/ScalarFromVector/Relabel/InfoJoin/Sample/Subquery -> their Promql*-prefixed names, ArithOp/CompareOp -> ArithmeticOpKind/ CompareOpKind) and reapplied mod.rs's doc/pub-use additions around the renamed expr_ir re-export. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Aug 22, 2026
…og (#225) (#233) Rebased onto main now that #227 (the catalog crate + generalized ClickHouse-builtin mechanism) and #226 (the QueryExpr rename) have landed there — this PR's own content is unchanged, just reapplied cleanly on top of current main instead of the now-deleted feat/sql-function-catalog-225 branch. Co-authored-by: Claude Sonnet 5 <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
1 participant
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.
Summary
Applies the rename table agreed in #183: renames several
QueryExprvariants (and two operator-kind types) whose names don't signal what they are or which language they're for, once you're not the person who just wrote them.DistinctDedupMerge(+QueryExprError::EmptyMerge)Concat(+EmptyConcat)WindowFuncSQLWindowFuncScalarPromqlScalarVectorFromScalar/ScalarFromVectorPromqlVectorFromScalar/PromqlScalarFromVectorRelabelPromqlRelabelInfoJoinPromqlInfoEnrichSamplePromqlSeriesSampleSubqueryPromqlSubqueryArith(bothQueryExpr::ArithandBinaryOpKind::Arith)ArithmeticArithOp/CompareOp(types)ArithmeticOpKind/CompareOpKindEvalTimeQueryTimestamp(per the issue author's follow-up comment, not the originally proposedTimeNow)ComparePure mechanical rename plus matching doc-comment/prose updates (including
docs/pre-asap-ir.md's "PromQL-specific nodes" section) — no behavior change.Two judgment calls made along the way, called out for review:
BinaryOpKind::Arithwas renamed alongsideQueryExpr::Aritheven though the approved table only said "Arith(variant)" without disambiguating — the two are cross-referenced in each other's doc comments as the same concept.QueryExprError::EmptyMerge→EmptyConcatwasn't explicitly in the table but is the error variant for the renamedMerge/Concatnode.DataFusion's own like-named types (
LogicalPlan::Distinct,Distinct::On/Distinct::All,Expr::Subquery) and a few unrelated same-named symbols elsewhere in the codebase (SummaryFamilyType::Sample,Implementation::Sample) were deliberately left untouched.Test plan
cargo build --workspace --all-targetscargo test --workspaceCloses #183
🤖 Generated with Claude Code