Add Claude Code GitHub Workflow - #1
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughTwo new GitHub Actions workflows are added to the repository. The first workflow, "Claude Code Review," automatically triggers on pull request creation or synchronization and runs a code review analysis evaluating code quality, bugs, performance, security, and test coverage. The second workflow, "Claude Code," triggers on issue comments, pull request review comments, issues creation, and pull request reviews, running the Claude Code action when activated by a specific trigger pattern in comments. Both workflows configure repository checkout and authenticate with provided tokens to enable the Claude integrations. ✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
Code Review: Claude Code GitHub Workflow IntegrationI've reviewed this PR that adds Claude Code integration via GitHub Actions workflows. Here's my detailed feedback: ✅ Code Quality & Best PracticesPositive aspects:
Recommendations:
🐛 Potential Issues
🔒 Security ConsiderationsGood security practices:
Concerns:
⚡ Performance Considerations
🧪 Test CoverageMissing aspects:
📋 Additional Recommendations
SummaryThis is a well-thought-out integration that follows GitHub Actions best practices. The main concerns are:
The code quality is good, but addressing the security and consistency issues would align it better with the Apache DataFusion Ballista project standards. Recommendation: Request changes to address security concerns and project conventions before merging. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read | ||
| pull-requests: read |
There was a problem hiding this comment.
Bug: Insufficient permissions prevent PR commenting functionality
The workflow prompt on line 52 instructs Claude to "Use gh pr comment with your Bash tool to leave your review as a comment on the PR", but the permissions on line 24 only grant pull-requests: read. The gh pr comment command requires pull-requests: write permission to create comments. This will cause the workflow to fail at runtime with a "Resource not accessible by integration" error when Claude attempts to post its review.
Additional Locations (1)
| (github.event_name == 'issue_comment' && contains(github.event.comment.body, '@claude')) || | ||
| (github.event_name == 'pull_request_review_comment' && contains(github.event.comment.body, '@claude')) || | ||
| (github.event_name == 'pull_request_review' && contains(github.event.review.body, '@claude')) || | ||
| (github.event_name == 'issues' && (contains(github.event.issue.body, '@claude') || contains(github.event.issue.title, '@claude'))) |
There was a problem hiding this comment.
Bug: Missing write-access check contradicts documented security model
The PR description states "Only users with write access to the repository can trigger the workflow" as a security feature, but the workflow's if condition only checks for @claude mentions without verifying the commenter's permissions. In public repositories, anyone who can comment can trigger Claude, potentially consuming API resources. The condition needs an additional check like github.event.comment.author_association being COLLABORATOR, MEMBER, or OWNER to enforce the documented security model.
…esolved stats (apache#1684) * feat: AQE rule to coalesce shuffle partitions (Spark-port) After an upstream shuffle stage finalizes with per-partition byte stats, this rule rewrites the downstream stage to read K < M logical partitions when several upstream partitions are near-empty. Mirrors Spark's CoalesceShufflePartitions for distributed query execution. The rule is opt-in (`ballista.coalesce.enabled` defaults to false). The only tunable is `ballista.coalesce.target_partition_bytes` (advisory, 64 MB default) — Spark's `advisoryPartitionSizeInBytes`. ## How it plugs into the AQE planner The rule is invoked **per stage** in `actionable_stages()`, right before each newly-runnable stage is handed to `BallistaAdapter`. It is deliberately NOT registered in `default_optimizers()` because that chain runs on the entire residual plan tree on every `replan_stages()` call, which causes two problems: 1. Cross-stage alignment groups. The walker collects "topmost ExchangeExecs", which spans multiple future stages' inputs into one alignment group. A K decision driven by stage N's byte distribution gets stamped onto stage N+1's input exchanges too. 2. Stale state across fires. `set_coalesce` uses interior mutability on a shared `Arc<ExchangeExec>`. A fire that bails on K=1 degenerate doesn't invalidate the K=5 a previous fire wrote on the same exchange — the next stage then sees mismatched leg Ks. Per-stage invocation fixes both: each call receives one stage's plan as its root, the walker descends to that stage's input exchanges only, and the K decision is local to that stage. The "unresolved leaf" bail path becomes unreachable by construction (a stage becomes runnable precisely when all its inputs are resolved). ## Example: TPC-H Q8 stage 11, both legs aligned After stage 10 finalizes (writes `Hash(o_custkey, 16)`), the rule fires on stage 11's plan: AdaptiveDatafusionExec ProjectionExec SortMergeJoinExec on (o_custkey, c_custkey) SortExec ExchangeExec Hash([o_custkey], 16) plan_id=11 <- leaf #1, resolved ...stage 10 writer output... SortExec ExchangeExec Hash([c_custkey], 16) plan_id=4 <- leaf #2, resolved ...customer table scan... Walker collects exactly 2 leaves: {plan_id=11, plan_id=4}. Per-leaf bytes are ~19 MB each across 16 partitions, summed [38M×16]. Bin-pack at 64 MB target → K=5. Both leaves get the SAME `CoalescePlan` (K=5, M=16, identical group mapping) → the SMJ runs with 5 partitions on each side, hash buckets stay aligned. ## Example: Q8 stage 12, three resolved siblings, K=1 degenerate When stage 11 finalizes and stage 12 is surfaced, the walker sees: leaves = [plan_id=12 (c_nationkey from stage 11), plan_id=5 (n_nationkey from stage 5), plan_id=6 (n_nationkey from stage 6), plan_id=7 (r_regionkey from stage 7)] All four resolved, but summed bytes total only ~76 MB (the multi-join filtered hard). Bin-pack returns K=1 → degenerate → the rule no-ops on this stage. The SMJ runs at native M=16 across all four legs. No partition-count mismatch. ## Components - Proto: `CoalescePlan` / `PartitionGroup` on `ShuffleReaderExecNode` and `UnresolvedShuffleExecNode` (field numbers 7/8 — non-clashing with PR apache#1647's `broadcast` / `upstream_partition_count` fields 5/6). - Reader: `ShuffleReaderExec::try_new_coalesced` builds the K-shape pre-concatenated reader; legacy `try_new` path byte-for-byte unchanged when no `CoalescePlan` is attached. Coexists with `try_new_broadcast` from PR apache#1647. - Algorithm: `split_size_list_by_target_size` is a verbatim Rust port of Spark's legacy `splitSizeListByTargetSize` — merged-factor early flush and small-tail post-loop merge preserved. - Rule: `CoalescePartitionsRule` (unit struct), invoked per-stage in `actionable_stages()`. ExchangeExec's display conditionally appends `coalesce=K of M` only when attached, so existing AQE snapshots that don't involve coalesce are unchanged. ## Behavior preservation When `coalesce.enabled=false`, the rule short-circuits as the first statement of `optimize()` and returns the input Arc verbatim. The reader path with no `CoalescePlan` attached is byte-for-byte identical to today's reader. Orthogonal to PR apache#1647 (broadcast hash join, merged) and PR apache#1649 (lazy AQE planner, merged). All three use DataFusion's standard `PhysicalOptimizerRule` trait. ## Validation - `cargo test --workspace`: ~600 tests pass, 0 failures. - `cargo clippy --workspace --all-targets --tests`: 0 warnings. - TPC-H SF100 sanity, 22 queries × 2 join variants (hash-pref and sort-pref), `coalesce.enabled=true`: 44/44 rc=0, row counts identical across variants, 9 queries get meaningful K reductions (K=2..K=13 across nation/region/supplier/customer joins). * refactor(coalesce): address PR review — namespace keys, Float64 storage - Rename ballista.coalesce.* → ballista.planner.coalesce.* to match the existing ballista.planner.* convention used for adaptive/broadcast knobs - Add DataType::Float64 to BallistaConfig::parse_value - Switch small/merged partition factors from Utf8 to Float64 storage - Extract get_float_setting helper mirroring get_usize_setting; drop the manual f64::from_str + unwrap_or fallback at the call sites - Document the neighboring-partitions-only grouping rule in the module doc * fix(coalesce): bail on heterogeneous M to avoid Q22 panic Guard against the byte-sum loop indexing past `summed.len()` when an alignment group contains leaf Exchanges with different partition_counts (e.g. Q22's scalar avg subquery at M=14 alongside hash-join legs at M=48). * style: cargo fmt the Q22 guard * refactor(coalesce): address PR review nits - Remove dead ShuffleReaderExec branch in BallistaAdapter — the coalesce rule never produces one (it only calls set_coalesce on the Exchange), so transform_children only ever sees ExchangeExec. - Restore PartitionStats.num_bytes to pub(crate); rule reads via the existing num_bytes() getter instead of direct field access.
2 participants
🤖 Installing Claude Code GitHub App
This PR adds a GitHub Actions workflow that enables Claude Code integration in our repository.
What is Claude Code?
Claude Code is an AI coding agent that can help with:
How it works
Once this PR is merged, we'll be able to interact with Claude by mentioning @claude in a pull request or issue comment.
Once the workflow is triggered, Claude will analyze the comment and surrounding context, and execute on the request in a GitHub action.
Important Notes
Security
There's more information in the Claude Code action repo.
After merging this PR, let's try mentioning @claude in a comment on any PR to get started!