Repository navigation
Refuse a ClickHouse GLOBAL membership test - #201
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Git Dependency Pin Stays Out Of Commits |  |
The PR modifies Cargo.lock and fuzz/Cargo.lock, while Cargo.toml, fuzz/Cargo.toml, and subql-demo/Cargo.toml declare git = dependencies without rev or tag (for example, Cargo.toml:26… |
Either revert the Cargo.lock and fuzz/Cargo.lock changes, or add an explicit rev or tag to every Git dependency in all repository manifests, then regenerate the lockfiles. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title is an imperative, plain statement of the main change. It is 42 characters long, has no conventional-commit prefix or file path, and has no trailing period. Docstring Coverage Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1 u… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. No Placeholder Implementations No added line matches the specified placeholder or deferral markers. The new unsupported_expression(expr)path explicitly rejectsGLOBALmembership as unsupported behavior; it is not a placeholder…No Blanket Diagnostic Suppression No added line matches a blanket diagnostic suppression. The Rust changes add explicit globalmatching, comments, and tests only; the lockfile and changelog additions contain no suppression directive…Behavior Change Carries A Test The pull request changes runtime behavior in src/compiler/parser/compilation_helpers.rs,src/compiler/prefilter.rs, andsrc/reexec/plan.rs. It also adds the#[test]item `a_global_membership_t…Crate Readme Is The Crate Documentation The check applies only when the pull request changes README.md or src/lib.rs. The authoritative diff changes neither file, although src/lib.rs exists. Pre-Alpha Has No Deployments No added line mentions a migration guide, migration SQL, upgrade path, existing deployments, version-bump coordination, flag day, rollout sequence, deprecation window, backward-compatibility shim, or … Prose Punctuation No added prose violates the punctuation rule. The changelog and Rust comments/doc comments contain no semicolons, em dashes, en dashes, curly quotes, or ellipses. The ASCII hyphens are list-marker or … Full details: Docstring Coverage
-
Explanation
Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 4 files. (1 skipped: 1 unsupported.)
Full details: No Placeholder Implementations
-
Explanation
No added line matches the specified placeholder or deferral markers. The new
unsupported_expression(expr)path explicitly rejectsGLOBALmembership as unsupported behavior; it is not a placeholder implementation. The added comments and changelog contain noTODO,FIXME,HACK, orXXXmarkers.
Full details: No Blanket Diagnostic Suppression
-
Explanation
No added line matches a blanket diagnostic suppression. The Rust changes add explicit
globalmatching, comments, and tests only; the lockfile and changelog additions contain no suppression directives.
Full details: Behavior Change Carries A Test
-
Explanation
The pull request changes runtime behavior in
src/compiler/parser/compilation_helpers.rs,src/compiler/prefilter.rs, andsrc/reexec/plan.rs. It also adds the#[test]itema_global_membership_test_is_refused_where_the_plain_one_compilesinsrc/compiler/parser/mod.rs, covering plain andGLOBALlist and subquery membership cases.
Full details: Pre-Alpha Has No Deployments
-
Explanation
No added line mentions a migration guide, migration SQL, upgrade path, existing deployments, version-bump coordination, flag day, rollout sequence, deprecation window, backward-compatibility shim, or an alias for old clients. The package is version 0.1.0 and the README links its crates.io package.
Full details: Prose Punctuation
-
Explanation
No added prose violates the punctuation rule. The changelog and Rust comments/doc comments contain no semicolons, em dashes, en dashes, curly quotes, or ellipses. The ASCII hyphens are list-marker or compound-word uses such as
right-handandBind-placeholder. Assert messages also comply.
-
Full details: Git Dependency Pin Stays Out Of Commits
-
Explanation
The PR modifies
Cargo.lockandfuzz/Cargo.lock, whileCargo.toml,fuzz/Cargo.toml, andsubql-demo/Cargo.tomldeclaregit =dependencies withoutrevortag(for example,Cargo.toml:26andCargo.toml:106). This breaks the invariant that changed lockfiles must not accompany bare Git dependencies.Resolution
Either revert the
Cargo.lockandfuzz/Cargo.lockchanges, or add an explicitrevortagto every Git dependency in all repository manifests, then regenerate the lockfiles.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #201 +/- ##
==========================================
+ Coverage 87.44% 87.46% +0.01%
==========================================
Files 162 162
Lines 48883 48926 +43
Branches 48883 48926 +43
==========================================
+ Hits 42744 42791 +47
+ Misses 4949 4940 -9
- Partials 1190 1195 +5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
apache/datafusion-sqlparser-rs#2598 added ClickHouse's
GLOBAL INandGLOBAL NOT IN, which put aglobalflag onExpr::InListandExpr::InSubquery. subql names every field of those variants in the compiler, the prefilter and the re-execution key predicate, so it stopped building against sqlparser main.GLOBALasks a distributed ClickHouse query to evaluate the right-hand side once on the initiator and broadcast it. None of subql's backends has that read, so the compiler refuses aGLOBALlist or subquery as it refuses any unsupported condition, the prefilter leaves it unknown, and the key predicate built for re-execution is neverGLOBAL. Only ClickHouse parses the keyword, so the test compiles ClickHouse-parsed conditions directly.The lockfiles follow sqlparser and sql-traits main in their own commits. The all-features, demo and fuzz builds stay red until LucaCappelletti94/pg2sqlite#122 merges, after which one more lock refresh lands here.
Adding
globaltosqlparser’s membership-expression variants broke compilation because the compiler, prefilter, and re-execution predicate did not account for the new field. The compiler now rejectsGLOBAL INandGLOBAL NOT INbecause the backend does not support ClickHouse’s distributed-query behavior.The prefilter treats global membership as unknown, and re-execution predicates remain non-global. ClickHouse-specific tests cover plain and global list and subquery membership conditions.