andygrove commented on code in PR #2387:
URL:
https://github.com/apache/datafusion-ballista/pull/2387#discussion_r3890038019
##########
ballista/core/src/extension.rs:
##########
@@ -1208,6 +1090,44 @@ mod test {
)
}
+ #[test]
+ fn should_round_trip_all_macro_generated_options() {
+ let plain = SessionConfig::new_with_ballista()
Review Comment:
`new_with_ballista()` installs the `BallistaConfig` extension, so this only
exercises the `if` branch of each generated setter. The `else` branch, where
the setter is called on a `SessionConfig` that has no ballista extension yet
and has to install one first, never runs. For a refactor whose whole claim is
that the generated code does what the hand-written code did, that is the half
worth covering. A second pass over a plain `SessionConfig::new()` would do it,
and the variable name here suggests that was the intent.
Small thing too, `ballista_shuffle_reader_force_remote_read()` is asserted
twice below, so one of those can go.
##########
ballista/core/src/config.rs:
##########
@@ -746,8 +752,8 @@ impl BallistaConfig {
/// Returns the target post-coalesce partition byte size in bytes
/// (Spark's `advisoryPartitionSizeInBytes`).
- pub fn coalesce_target_partition_bytes(&self) -> u64 {
- self.get_usize_setting(BALLISTA_COALESCE_TARGET_PARTITION_BYTES) as u64
+ pub fn coalesce_target_partition_bytes(&self) -> usize {
Review Comment:
This one is a hard break with no deprecation path, which makes the PR a bit
inconsistent with itself. `default_standalone_parallelism` and `client_use_tls`
both got a `#[deprecated]` alias a few hundred lines up, but this changes a
public return type in place.
It could have been additive. Either keep a deprecated `u64`-returning shim
under the old name and add the new one, or just leave this returning `u64` and
let the macro-generated trait method do the cast. Happy either way as long as
it is a deliberate call rather than a side effect of lining the types up for
the macro, but if it stays as is then it belongs in the upgrade guide alongside
the trait change.
##########
Cargo.lock:
##########
@@ -6977,7 +6984,7 @@ source =
"registry+https://github.com/rust-lang/crates.io-index";
checksum = "32497e9a4c7b38532efcdebeef879707aa9f794296a4f0244f6f69e9bc8574bd"
dependencies = [
"fastrand",
- "getrandom 0.3.4",
+ "getrandom 0.4.3",
Review Comment:
This `getrandom` bump under `tempfile` looks like incidental lockfile regen
churn rather than anything `pastey` pulled in. Probably cleaner to drop it from
the diff so the lockfile change is just the new dependency.
##########
ballista/core/src/extension.rs:
##########
@@ -277,9 +277,9 @@ pub trait SessionConfigExt {
fn with_ballista_coalesce_enabled(self, enabled: bool) -> Self;
/// Returns the target post-coalesce partition byte size in bytes.
- fn ballista_coalesce_target_partition_bytes(&self) -> u64;
+ fn ballista_coalesce_target_partition_bytes(&self) -> usize;
/// Sets the target post-coalesce partition byte size in bytes.
- fn with_ballista_coalesce_target_partition_bytes(self, bytes: u64) -> Self;
+ fn with_ballista_coalesce_target_partition_bytes(self, bytes: usize) ->
Self;
Review Comment:
`SessionConfigExt` is a public trait, so re-signing a method breaks external
implementors and not just callers. I think this one is unavoidable, you cannot
have two trait methods with the same name and different types, and inventing a
new name just to dodge it would be worse. So I am fine taking the break here.
It does need an entry in `docs/source/upgrading/55.0.0.md` though, under the
existing "API changes" heading. Something short saying these two moved from
`u64` to `usize` and callers passing an explicit `u64` will need a cast.
##########
ballista/core/src/extension.rs:
##########
@@ -373,6 +373,80 @@ impl SessionStateExt for SessionState {
}
}
+/// Calls the `SessionConfig` setter matching `$ty`, converting `$val` first
+/// when the type needs it (only `f64` does, via `set_str`/`to_string()`,
+/// since `SessionConfig` has no `set_f64`).
+macro_rules! ballista_set_scalar {
+ (bool, $self:expr, $const:expr, $val:expr) => {
+ $self.set_bool($const, $val)
+ };
+ (usize, $self:expr, $const:expr, $val:expr) => {
+ $self.set_usize($const, $val)
+ };
+ (u64, $self:expr, $const:expr, $val:expr) => {
+ $self.set_u64($const, $val)
+ };
+ (f64, $self:expr, $const:expr, $val:expr) => {
+ $self.set_str($const, &$val.to_string())
+ };
+}
+
Review Comment:
Worth calling out in this doc comment that the macro now couples two public
naming schemes. Because the trait method name is derived from the
`BallistaConfig` method name, `BallistaConfig`'s public getters and
`SessionConfigExt`'s methods can no longer be renamed independently. That is
what forced the `default_standalone_parallelism` and `client_use_tls` renames,
and it is a reasonable trade, but the next person renaming a config getter
should know they are also renaming a public trait method.
The `as <setter_name>` escape hatch already covers the one case where the
two diverge, which is a nice touch.
--
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]
