Core: Set scan planning mode when initializing client#15903
Conversation
|
cc @singhpk234 Also, in the ScanPlanningMode enum it felt an overkill to have the fromString and modeName functions, because the enumerator itself already has the required functionality for this. Let me know if you disagree and I'll revert. |
gaborkaszab
force-pushed
the
main_scan_planning_mode_setting
branch
from
e8ca037 to
a2e8f33
Compare
| public String modeName() { | ||
| return name().toLowerCase(Locale.ROOT); | ||
| } | ||
|
|
||
| public static ScanPlanningMode fromString(String mode) { | ||
| for (ScanPlanningMode planningMode : values()) { | ||
| if (planningMode.modeName().equalsIgnoreCase(mode)) { | ||
| return planningMode; | ||
| } | ||
| } | ||
|
|
||
| throw new IllegalArgumentException( | ||
| String.format( | ||
| "Invalid scan planning mode: %s. Valid values are: %s", | ||
| mode, | ||
| Arrays.stream(values()) | ||
| .map(ScanPlanningMode::modeName) | ||
| .collect(Collectors.joining(", ")))); | ||
| } |
There was a problem hiding this comment.
we are changing the exception message by removing this, i would much rather have the possible values defined to prompt the user to correct message.
There was a problem hiding this comment.
Thanks for spotting this! My initial intention was to rather start a conversation on this, but I forgot to write the comment.
- In general I don't think we print the available values when we can't initialize an enum. These values are in the docs, seems an overkill to also return here.
- Instead of having a for loop of values, we can just simply use the
valueOfof the enum. This simplifies thefromStringimplementation.
I went back to use the fromString function because it can hide some details, like calling uppercase, and this way we can have a custom error message instead of the one from the enum. I felt printing the available values is an overkill, and also went for the simplification in 2)
Taking a step back, the changes on the enum seems orthogonal to the original intent of the PR, so I can split that into a separate one (and also tackle the same for SnapshotMode).
WDYT @singhpk234
There was a problem hiding this comment.
This is somewhat unrelated to the scope of the PR. May I move this into a separate PR and continue the discussion there?
| // Determine effective mode: prefer server config if present, otherwise use client config, | ||
| // fall back to default if both are null | ||
| ScanPlanningMode effectiveMode = | ||
| scanPlanningModeServer != null | ||
| ? scanPlanningModeServer | ||
| : scanPlanningMode != null | ||
| ? scanPlanningMode | ||
| : RESTCatalogProperties.SCAN_PLANNING_MODE_DEFAULT; |
There was a problem hiding this comment.
can we still maintain the 2 if ? it hard to follow a ternary within ternary op
There was a problem hiding this comment.
Sure, changed this to the 2-if approach
gaborkaszab
force-pushed
the
main_scan_planning_mode_setting
branch
4 times, most recently
from
ec086d5 to
4e42a5b
Compare
gaborkaszab
force-pushed
the
main_scan_planning_mode_setting
branch
from
4e42a5b to
3f16e42
Compare
|
Hey @singhpk234 , |
|
Gentle reminder @singhpk234 :) Do you think this version of the PR is ok? |
Since it's a startup parameter for RESTSessionCatalog, setting client-side scan planning mode is move to the initialize() function. With this we can throw an exception right away when an invalid value is set, not just when we try to load a table.
gaborkaszab
force-pushed
the
main_scan_planning_mode_setting
branch
from
3f16e42 to
aec9b03
Compare
gaborkaszab
changed the title
|
Hey @singhpk234 , |
2 participants
Since it's a startup parameter for RESTSessionCatalog, setting client-side scan planning mode is move to the initialize() function. With this we can throw an exception right away when an invalid value is set, not just when we try to load a table.