Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10205#issuecomment-197156120
@vanzin I finally had some time to look over this change. I like the
direction this is going to apply more semantics for configs, but I find the
pre-existing SQLConf much easier to understand as its class structure is much
simpler.
A few questions/comments:
- What's the difference between OptionalConfigEntry and something that has
default value null? I only find OptionalConfigEntry used in the setter, but
nothing specific to a getter.
- Why is the builder pattern better than just a ctor with default argument
values?
- Function naming is inconsistent. E.g. "withDefault" and "doc" vs "withDoc"
- The convention is for state mutation functions (e.g. internal) to have
parentheses. Right now they look like some variable that's being accessed.
- Do we really need all these classes? Even if we use the builder pattern,
I'd imagine we can live with only two classes, a builder and a config.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
