JarroVGIT opened a new pull request, #1852:
URL: https://github.com/apache/datafusion-ballista/pull/1852
# Which issue does this PR close?
Draft PR.
Closes #.
# Rationale for this change
`job_id` and `job_name` (and also, `session_id` and `executor_id`) are all
`String` and `&str` throughout the code base. It can make it hard to reason
about HashMaps that are keyed by such identifier (`HashMap<String, SomeType>`
is not obvious in what it stores). It also makes it easy to make a mistake when
passing such items to functions that expect a few of them. For example `fn
function(job_name: &str, job_id: &str) -> ()` is valid if called with reversed
arguments.
NewType also brings new code and overhead. This PR is created as a draft to
see how it would look like if implemented for `job_id` and `job_name`.
# What changes are included in this PR?
- Notably: the wire format remains unchanged (e.g. the generated code still
expects `String`s).
- Created the newtypes (through a macro that also implements all the nice
traits, such as `Display`, `From<String>`, `From<&str>`, `From<JobId> for
String` (for unwrapping back into proto structs), `AsRef<str>` and
`Borrow<str>` (which allows for `map.get("job-123")` without allocating a JobId
per lookup.
- Replaced all `job_name: String` and `job_id: String` in internal structs
(non-proto)
- Followed the compiler and fixed the errors.
# Are there any user-facing changes?
I am going to assume there will be, yes, but I haven't paid any attention to
it yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
