1852: NewType pattern for job_id and job_name - #71
Conversation
…rom `ballista_core`
…&)JobName` in scheduler (untouched: core/executor/cli)
…` and all function parameters.
…JobId` in core/executor/scheduler.
WalkthroughThis pull request migrates Ballista from using plain ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces strongly-typed string identifiers, JobId and JobName, to replace bare String and &str types across the Ballista codebase, significantly improving type safety. The feedback highlights critical correctness bugs in the Adaptive Query Execution (AQE) planner where JobName is incorrectly converted to JobId, which would break EXPLAIN queries and cause shuffle path mismatches. Additionally, a performance improvement is suggested to use unwrap_or_else instead of unwrap_or to avoid eager string allocation on the happy path.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Note: the signature requires a JobId, but we are passing a JobName. The below is a | ||
| // dirty fix but this seems like a bug or a design flaw. | ||
| let job_id: JobId = job_name.clone().into_inner().into(); | ||
| let plan = handle_explain_plan(&job_id, ctx, logical_plan, plan) |
There was a problem hiding this comment.
Critical Correctness Bug & Type Safety Violation
Converting JobName to JobId here is not just a design flaw; it is a critical correctness bug that will cause EXPLAIN queries to fail or behave incorrectly at runtime under Adaptive Query Execution (AQE).
Why this is a bug:
handle_explain_plancallsgenerate_distributed_explain_plan, which uses the passedJobIdto look up the job's execution graph and stages from the scheduler's state.- Jobs are registered in the scheduler using their actual unique
JobId(generated viagenerate_job_id()), not their non-unique displayJobName. - By passing a
JobNamemasquerading as aJobId, any state lookup for the job will fail (returningNoneor an error), completely breaking the distributed explain plan generation.
Solution:
Refactor AdaptivePlanner to accept and store both JobId and JobName. Since AdaptiveExecutionGraph::try_new already has access to the real JobId, you can easily pass it down:
- Update
AdaptivePlannerstruct to holdjob_id: JobId. - Update
AdaptivePlanner::try_newsignature to acceptjob_id: JobId. - Pass the real
job_idfromAdaptiveExecutionGraph::try_newwhen constructing the planner. - Update the test cases to pass both a dummy
JobIdandJobName.
let plan = handle_explain_plan(&job_id, ctx, logical_plan, plan)There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! There is a bug here where the job name is used as a job id. Using the new custom types exposed this issue! Prevents a bug when one identifier is used instead of another one.
| // adapt_to_ballista takes an job_id, we are passing a job_name. Need to transform to fix compiler. | ||
| let job_id = self.job_name.clone().into_inner().into(); | ||
| BallistaAdapter::adapt_to_ballista(plan, &job_id, config) |
There was a problem hiding this comment.
Critical Correctness Bug: Shuffle Path Mismatch
Bypassing the type system here by converting self.job_name to a JobId introduces a critical runtime bug in shuffle data exchange for AQE queries.
Why this is a bug:
BallistaAdapter::adapt_to_ballistauses the passedJobIdto plan shuffle stages.- These shuffle stages write their output files to disk using a path generated by
create_shuffle_path(work_dir, job_id, ...). - If you pass the
job_name(masquerading as aJobId) here, the shuffle writer will write files to a directory named after the job name (e.g.,/work/my_job_name/...). - However, the rest of the system (including the shuffle readers and cleanup tasks) will attempt to read/manage these files using the actual unique job ID (e.g.,
/work/job_123/...). - This mismatch will result in immediate
FileNotFounderrors and broken query execution during shuffle exchange.
Solution:
Once AdaptivePlanner is refactored to store the actual job_id: JobId (as suggested in the other comment), replace this conversion with &self.job_id.
BallistaAdapter::adapt_to_ballista(plan, &self.job_id, config)There was a problem hiding this comment.
value:useful; category:bug; feedback: The Gemini AI reviewer is correct! There is a bug here where the job name is used as a job id. Using the new custom types exposed this issue! Prevents a bug when one identifier is used instead of another one.
| let job_id: JobId = status | ||
| .as_ref() | ||
| .map(|s| s.job_id.to_owned()) | ||
| .unwrap_or("unknown_job_id".to_string()); // should not happen | ||
| if !job_id.starts_with("unknown_") { | ||
| .unwrap_or("unknown_job_id".to_string()) // should not happen | ||
| .into(); |
There was a problem hiding this comment.
Performance/Efficiency Issue: Eager String Allocation
Using unwrap_or with a heap-allocating function like to_string() causes an unnecessary allocation on the happy path (when status is Some), because unwrap_or eagerly evaluates its argument.
Solution:
Use unwrap_or_else to lazily evaluate the fallback string only when status is None.
| let job_id: JobId = status | |
| .as_ref() | |
| .map(|s| s.job_id.to_owned()) | |
| .unwrap_or("unknown_job_id".to_string()); // should not happen | |
| if !job_id.starts_with("unknown_") { | |
| .unwrap_or("unknown_job_id".to_string()) // should not happen | |
| .into(); | |
| let job_id: JobId = status | |
| .as_ref() | |
| .map(|s| s.job_id.to_owned()) | |
| .unwrap_or_else(|| "unknown_job_id".to_string()) | |
| .into(); |
There was a problem hiding this comment.
value:good-to-have; category:bug; feedback: The Gemini AI reviewer is correct! There is no need to allocate a String unconditionally. It should be allocated only as a fallback. Prevents useless memory allocation and deallocation.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5007dd1. Configure here.
| // Note: the signature requires a JobId, but we are passing a JobName. The below is a | ||
| // dirty fix but this seems like a bug or a design flaw. | ||
| let job_id: JobId = job_name.clone().into_inner().into(); | ||
| let plan = handle_explain_plan(&job_id, ctx, logical_plan, plan) |
There was a problem hiding this comment.
AQE explain uses job name
Medium Severity
Adaptive AQE planning now calls handle_explain_plan with a JobId built from JobName, while the static planner passes the real scheduler JobId. When the display name differs from the assigned id (typical with BALLISTA_JOB_NAME), distributed EXPLAIN output is planned under the wrong identifier.
Reviewed by Cursor Bugbot for commit 5007dd1. Configure here.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The Bugbot AI reviewer is correct! There is a bug here where the job name is used as a job id. Using the new custom types exposed this issue! Prevents a bug when one identifier is used instead of another one.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ballista/executor/src/executor_server.rs (1)
904-920:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate
cancel_task == falseinto the RPC response.This loop only flips
cancelledonErr, soCancelTasksResult { cancelled: true }is still returned whenExecutor::cancel_taskreportsOk(false)for a missing task handle. That makes the RPC over-report successful cancellations.Suggested fix
- for task in task_infos { - if let Err(e) = self - .executor - .cancel_task( - task.task_id as usize, - task.job_id.into(), - task.stage_id as usize, - task.partition_id as usize, - ) - .await - { - error!("Error cancelling task: {e:?}"); - cancelled = false; - } - } + for task in task_infos { + match self + .executor + .cancel_task( + task.task_id as usize, + task.job_id.into(), + task.stage_id as usize, + task.partition_id as usize, + ) + .await + { + Ok(true) => {} + Ok(false) => cancelled = false, + Err(e) => { + error!("Error cancelling task: {e:?}"); + cancelled = false; + } + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/executor/src/executor_server.rs` around lines 904 - 920, The loop calling self.executor.cancel_task currently treats only Err as failure, so Ok(false) (no task handle found) is reported as success; update the logic in the loop that iterates over task_infos (where executor.cancel_task(...) is awaited) to inspect the Result<bool, _> return: if it is Err(_) or Ok(false) set cancelled = false (and optionally log a warning mentioning the task IDs), otherwise keep cancelled true; ensure the final Response::new(CancelTasksResult { cancelled }) reflects any Ok(false) outcomes from cancel_task.ballista/core/src/execution_plans/sort_shuffle/spill.rs (1)
75-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-path-safe
JobIdvalues before using them in spill paths.Line 84 joins
job_id.as_str()directly intowork_dir, but this PR still showsJobId::new(""),".", and".."being constructible inballista/executor/src/executor_process.rstests. A malformed job id can therefore escape the executor work dir here, andcleanup()will laterremove_dir_allon that escaped path. Please validateJobIdas a single normal path component once, or encode it before any filesystem join (SpillManager,finalize_output, and shared shuffle-path helpers).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/core/src/execution_plans/sort_shuffle/spill.rs` around lines 75 - 89, The spill path builder (SpillManager::new / function new in spill.rs) currently joins job_id.as_str() into work_dir without validation, allowing JobId values like "" or "."/".." to escape the work directory; fix by validating the JobId once before any filesystem joins (in SpillManager::new and the related finalize_output and shared shuffle-path helpers): ensure JobId.as_str() is non-empty, not "." or "..", contains no path separators or traversal components, and is a single normal path component (or alternatively replace/encode it into a safe token), and return an error if validation fails so no unsafe path is created; update callers (cleanup/remove_dir_all) to rely on the validated/encoded JobId.
🧹 Nitpick comments (5)
ballista/executor/src/execution_loop.rs (1)
232-298: ⚡ Quick winConsider converting
job_idonce to avoid repeated conversions.The current code extracts
job_idfrom the task at line 234 and then converts it using.clone().into()at lines 278 and 288. This performs the conversion twice. Consider converting once at line 234:let job_id: JobId = task.job_id.into();Then use
job_id.clone()at lines 278 and 288. This reduces the overhead of repeated conversions and makes the intent clearer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/executor/src/execution_loop.rs` around lines 232 - 298, The code repeatedly converts task.job_id with `.clone().into()`; change the initial binding to convert once (e.g., bind `job_id` as the target JobId type from `task.job_id.into()`), then update uses in `create_query_stage_exec` and `PartitionId`/`part` construction to use `job_id.clone()` (remove the `.into()` there) so conversion happens only once; adjust any type expectations around `job_id` in `create_query_stage_exec` and `PartitionId` creation to match the converted type.ballista/core/src/execution_plans/shuffle_reader.rs (1)
1021-1021: 💤 Low valueConsider simplifying
JobIdconversions in tests.The pattern
job_id.to_owned().into()(wherejob_idis&str) can be shortened to justjob_id.into()becauseJobIdimplementsFrom<&str>(line 90-94 inids.rs). Similarly,"literal".to_owned().into()can be"literal".into()orJobId::new("literal").♻️ Example simplification
partition_id: PartitionId { - job_id: job_id.to_owned().into(), + job_id: job_id.into(), stage_id: input_stage_id,Also applies to: 1140-1140, 1191-1191, 1243-1243, 1295-1295
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/core/src/execution_plans/shuffle_reader.rs` at line 1021, Replace unnecessary to_owned().into() conversions when constructing JobId in tests with direct into() (or JobId::new) since JobId implements From<&str> (see ids.rs lines ~90-94); specifically update occurrences like job_id: job_id.to_owned().into() and "literal".to_owned().into() in shuffle_reader.rs (the instances around the previously noted spots) to job_id.into() or JobId::new("literal") to simplify and remove the extra allocation.ballista/scheduler/src/api/handlers.rs (1)
111-113: ⚡ Quick winAdd a regression test for the REST JSON shape.
JobResponsenow exposesJobIdandJobNamedirectly. A small serialization test here would lock the API to plain JSON strings and catch any future loss of transparent serde behavior before REST clients start receiving a different payload shape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/scheduler/src/api/handlers.rs` around lines 111 - 113, Add a regression test that serializes and deserializes JobResponse to ensure JobId and JobName remain plain JSON strings: create a test (e.g., in the same module tests or a new unit test in handlers.rs) that constructs a JobResponse with sample JobId and JobName, serializes it to JSON, asserts the JSON shape contains simple string values for those fields, then deserializes back and asserts equality; reference the JobResponse, JobId, and JobName types to locate the code and lock the REST JSON shape via serde behavior.ballista/scheduler/src/scheduler_server/event.rs (1)
153-155: ⚡ Quick winRename the
TaskUpdatingdebug field toexecutor_id.The variant carries an executor ID, but the formatter still logs it as
job_id. Now that actualJobIdvalues are typed in the other variants, this line becomes misleading during scheduler incident triage.Suggested change
- // TODO: This is not job_id but Executor ID (based on usage). - QueryStageSchedulerEvent::TaskUpdating(job_id, status) => { - write!(f, "TaskUpdating : job_id={job_id}, status:[{status:?}].") + QueryStageSchedulerEvent::TaskUpdating(executor_id, status) => { + write!( + f, + "TaskUpdating : executor_id={executor_id}, status:[{status:?}]." + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/scheduler/src/scheduler_server/event.rs` around lines 153 - 155, The formatter for QueryStageSchedulerEvent::TaskUpdating currently logs the first field as job_id but that field is actually an Executor ID; update the debug message in the Display/Debug implementation where QueryStageSchedulerEvent::TaskUpdating(job_id, status) is matched (the write!(f, ...) call) to use a clearer name like executor_id and adjust the message to "TaskUpdating : executor_id={executor_id}, status:[{status:?}]." so it no longer mislabels the value.ballista/scheduler/src/state/aqe/mod.rs (1)
181-182: 💤 Low valuePrefer consistent conversion pattern for JobStatus construction.
The constructor at lines 181–182 uses
.to_string()to convert&JobIdand&JobNameintoStringfields forJobStatus, while thefail_jobandsucceed_jobmethods at lines 1179–1180 and 1208–1209 use.clone().into()for the same conversion. Prefer a single pattern—either.to_string()everywhere or.into()everywhere—for consistency and clarity.Also applies to: 1179-1180, 1208-1209
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ballista/scheduler/src/state/aqe/mod.rs` around lines 181 - 182, The JobStatus struct is being populated with String fields using different conversion styles; unify them for consistency by changing the conversions in the JobStatus construction and in the fail_job and succeed_job methods to the same pattern (e.g., replace .to_string() with .clone().into() or vice versa). Update the occurrences where job_id and job_name are converted in the JobStatus creation (the code that sets job_id: ... and job_name: ...) and the conversions inside the fail_job and succeed_job methods so all three locations use the identical conversion method (reference JobStatus, fail_job, and succeed_job to locate the code).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ballista/scheduler/src/state/aqe/planner.rs`:
- Around line 169-172: The conversion job_name.clone().into_inner().into() in
planner.rs is unsafe/unclear because we don't know whether JobName and JobId are
compatible; inspect the type definitions and conversion impls for JobName,
JobId, and any into_inner()/From/Into impls (search for JobName, JobId,
into_inner, and impl From for these types) and then fix by either (a) using the
correct identifier type expected by handle_explain_plan(JobId) by providing an
explicit, well-named conversion function (e.g., JobName::to_job_id) or
implementing a clear From<JobName> for JobId with tests, or (b) changing
handle_explain_plan to accept JobName if that is the intended semantics; update
the call site (job_name → proper conversion) and add a comment or unit test that
documents the conversion invariants.
---
Outside diff comments:
In `@ballista/core/src/execution_plans/sort_shuffle/spill.rs`:
- Around line 75-89: The spill path builder (SpillManager::new / function new in
spill.rs) currently joins job_id.as_str() into work_dir without validation,
allowing JobId values like "" or "."/".." to escape the work directory; fix by
validating the JobId once before any filesystem joins (in SpillManager::new and
the related finalize_output and shared shuffle-path helpers): ensure
JobId.as_str() is non-empty, not "." or "..", contains no path separators or
traversal components, and is a single normal path component (or alternatively
replace/encode it into a safe token), and return an error if validation fails so
no unsafe path is created; update callers (cleanup/remove_dir_all) to rely on
the validated/encoded JobId.
In `@ballista/executor/src/executor_server.rs`:
- Around line 904-920: The loop calling self.executor.cancel_task currently
treats only Err as failure, so Ok(false) (no task handle found) is reported as
success; update the logic in the loop that iterates over task_infos (where
executor.cancel_task(...) is awaited) to inspect the Result<bool, _> return: if
it is Err(_) or Ok(false) set cancelled = false (and optionally log a warning
mentioning the task IDs), otherwise keep cancelled true; ensure the final
Response::new(CancelTasksResult { cancelled }) reflects any Ok(false) outcomes
from cancel_task.
---
Nitpick comments:
In `@ballista/core/src/execution_plans/shuffle_reader.rs`:
- Line 1021: Replace unnecessary to_owned().into() conversions when constructing
JobId in tests with direct into() (or JobId::new) since JobId implements
From<&str> (see ids.rs lines ~90-94); specifically update occurrences like
job_id: job_id.to_owned().into() and "literal".to_owned().into() in
shuffle_reader.rs (the instances around the previously noted spots) to
job_id.into() or JobId::new("literal") to simplify and remove the extra
allocation.
In `@ballista/executor/src/execution_loop.rs`:
- Around line 232-298: The code repeatedly converts task.job_id with
`.clone().into()`; change the initial binding to convert once (e.g., bind
`job_id` as the target JobId type from `task.job_id.into()`), then update uses
in `create_query_stage_exec` and `PartitionId`/`part` construction to use
`job_id.clone()` (remove the `.into()` there) so conversion happens only once;
adjust any type expectations around `job_id` in `create_query_stage_exec` and
`PartitionId` creation to match the converted type.
In `@ballista/scheduler/src/api/handlers.rs`:
- Around line 111-113: Add a regression test that serializes and deserializes
JobResponse to ensure JobId and JobName remain plain JSON strings: create a test
(e.g., in the same module tests or a new unit test in handlers.rs) that
constructs a JobResponse with sample JobId and JobName, serializes it to JSON,
asserts the JSON shape contains simple string values for those fields, then
deserializes back and asserts equality; reference the JobResponse, JobId, and
JobName types to locate the code and lock the REST JSON shape via serde
behavior.
In `@ballista/scheduler/src/scheduler_server/event.rs`:
- Around line 153-155: The formatter for QueryStageSchedulerEvent::TaskUpdating
currently logs the first field as job_id but that field is actually an Executor
ID; update the debug message in the Display/Debug implementation where
QueryStageSchedulerEvent::TaskUpdating(job_id, status) is matched (the write!(f,
...) call) to use a clearer name like executor_id and adjust the message to
"TaskUpdating : executor_id={executor_id}, status:[{status:?}]." so it no longer
mislabels the value.
In `@ballista/scheduler/src/state/aqe/mod.rs`:
- Around line 181-182: The JobStatus struct is being populated with String
fields using different conversion styles; unify them for consistency by changing
the conversions in the JobStatus construction and in the fail_job and
succeed_job methods to the same pattern (e.g., replace .to_string() with
.clone().into() or vice versa). Update the occurrences where job_id and job_name
are converted in the JobStatus creation (the code that sets job_id: ... and
job_name: ...) and the conversions inside the fail_job and succeed_job methods
so all three locations use the identical conversion method (reference JobStatus,
fail_job, and succeed_job to locate the code).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 708b474e-2356-4e3f-ae98-64fe51d6c963
📒 Files selected for processing (54)
.cursor/rules.md.gemini/rules.mdAGENTS.mdCLAUDE.mdballista/core/src/execution_plans/distributed_explain_analyze.rsballista/core/src/execution_plans/distributed_query.rsballista/core/src/execution_plans/mod.rsballista/core/src/execution_plans/shuffle_reader.rsballista/core/src/execution_plans/shuffle_writer.rsballista/core/src/execution_plans/shuffle_writer_trait.rsballista/core/src/execution_plans/sort_shuffle/spill.rsballista/core/src/execution_plans/sort_shuffle/writer.rsballista/core/src/ids.rsballista/core/src/lib.rsballista/core/src/serde/mod.rsballista/core/src/serde/scheduler/from_proto.rsballista/core/src/serde/scheduler/mod.rsballista/core/src/serde/scheduler/to_proto.rsballista/executor/src/execution_engine.rsballista/executor/src/execution_loop.rsballista/executor/src/executor.rsballista/executor/src/executor_process.rsballista/executor/src/executor_server.rsballista/executor/src/lib.rsballista/executor/src/metrics/mod.rsballista/scheduler/src/api/handlers.rsballista/scheduler/src/cluster/memory.rsballista/scheduler/src/cluster/mod.rsballista/scheduler/src/cluster/test_util/mod.rsballista/scheduler/src/display.rsballista/scheduler/src/metrics/mod.rsballista/scheduler/src/metrics/prometheus.rsballista/scheduler/src/planner.rsballista/scheduler/src/scheduler_server/event.rsballista/scheduler/src/scheduler_server/grpc.rsballista/scheduler/src/scheduler_server/mod.rsballista/scheduler/src/scheduler_server/query_stage_scheduler.rsballista/scheduler/src/state/aqe/adapter.rsballista/scheduler/src/state/aqe/mod.rsballista/scheduler/src/state/aqe/planner.rsballista/scheduler/src/state/aqe/test/alter_stages.rsballista/scheduler/src/state/aqe/test/coalesce_rule.rsballista/scheduler/src/state/aqe/test/join_selection.rsballista/scheduler/src/state/aqe/test/mod.rsballista/scheduler/src/state/aqe/test/plan_to_stages.rsballista/scheduler/src/state/distributed_explain.rsballista/scheduler/src/state/execution_graph.rsballista/scheduler/src/state/execution_graph_dot.rsballista/scheduler/src/state/executor_manager.rsballista/scheduler/src/state/mod.rsballista/scheduler/src/state/task_manager.rsballista/scheduler/src/test_utils.rsbenchmarks/benches/sort_shuffle.rsbenchmarks/src/bin/shuffle_bench.rs
| // Note: the signature requires a JobId, but we are passing a JobName. The below is a | ||
| // dirty fix but this seems like a bug or a design flaw. | ||
| let job_id: JobId = job_name.clone().into_inner().into(); | ||
| let plan = handle_explain_plan(&job_id, ctx, logical_plan, plan) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check the definitions of JobId and JobName in ballista_core to determine if they wrap different underlying types or constraints.
rg -A 10 'pub struct JobId' ballista/core/src/ids.rs
rg -A 10 'pub struct JobName' ballista/core/src/ids.rsRepository: martin-augment/datafusion-ballista
Length of output: 60
Clarify the JobName → JobId conversion
The conversion in ballista/scheduler/src/state/aqe/planner.rs (job_name.clone().into_inner().into()) is only safe if JobName and JobId are semantically interchangeable (same underlying identifier type / compatible into() semantics). The prior check didn’t find pub struct JobId/pub struct JobName in ballista/core/src/ids.rs, so the definitions may live elsewhere or be type aliases/newtypes with different semantics.
I need the exact JobId and JobName type definitions (and any From/Into impls or into_inner() details) to confirm whether this conversion is correct or a bug/design flaw.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ballista/scheduler/src/state/aqe/planner.rs` around lines 169 - 172, The
conversion job_name.clone().into_inner().into() in planner.rs is unsafe/unclear
because we don't know whether JobName and JobId are compatible; inspect the type
definitions and conversion impls for JobName, JobId, and any
into_inner()/From/Into impls (search for JobName, JobId, into_inner, and impl
From for these types) and then fix by either (a) using the correct identifier
type expected by handle_explain_plan(JobId) by providing an explicit, well-named
conversion function (e.g., JobName::to_job_id) or implementing a clear
From<JobName> for JobId with tests, or (b) changing handle_explain_plan to
accept JobName if that is the intended semantics; update the call site (job_name
→ proper conversion) and add a comment or unit test that documents the
conversion invariants.
There was a problem hiding this comment.
value:useful; category:bug; feedback: The CodeRabbit AI reviewer is correct! There is a bug here where the job name is used as a job id. Using the new custom types exposed this issue! Prevents a bug when one identifier is used instead of another one.
3 participants
1852: To review by AI