You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@polaris.apache.org by "eric-maynard (via GitHub)" <gi...@apache.org> on 2025/04/02 20:29:37 UTC

[PR] Make PolarisConfiguration values optional [polaris]

eric-maynard opened a new pull request, #1295:
URL: https://github.com/apache/polaris/pull/1295

Created this PR to get feedback on an idea I had while trying to add a new configuration where we'd like to distinguish between "set" and "not set" without just setting the default value to `null` (which won't work for some types).

I don't love that this implementation requires `get` to be spammed everywhere. With that said, I think the current way of getting configs via the lengthy invocation `callContext.getPolarisCallContext().getConfigurationStore().getConfiguration(callContext.getPolarisCallContext, FeatureConfiguration.FEATURE_NAME)` should probably be made more ergonomic anyway and that adding `get` to the end doesn't make it too much more painful. We could potentially refactor out the `get`s if/when we refactor the rest. I leave them in here so reviewers can get a good sense of the scope of the change.

Critically, this implementation does mean that the caller of `getConfiguration` is expected to know when it's safe to use `get`, i.e. when a config is optional or not.

An alternative I considered is to make some new type like `OptionalConfiguation` and to provide new overrides of the `getConfiguration`-like methods for this new type. In that way, the caller could be somewhat shielded from having to worry about whether a config is optional. However, the config code is already pretty gnarly with multiple methods apparently doing the same thing, and I'm worried about adding more complexity there.

--
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Make PolarisConfiguration values optional [polaris]

Posted by "collado-mike (via GitHub)" <gi...@apache.org>.

collado-mike commented on PR #1295:
URL: https://github.com/apache/polaris/pull/1295#issuecomment-2795303734

   > I don't love that this implementation requires `get` to be spammed everywhere.
   
   I don't love that either. I also don't really love configurations that don't provide a default value - at least an empty string or empty list or -1 or something. Is there some specific case driving this change?


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Make PolarisConfiguration values optional [polaris]

Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.

eric-maynard closed pull request #1295: Make PolarisConfiguration values optional
URL: https://github.com/apache/polaris/pull/1295


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Make PolarisConfiguration values optional [polaris]

Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.

eric-maynard commented on PR #1295:
URL: https://github.com/apache/polaris/pull/1295#issuecomment-2795840508

@collado-mike

For int options, `-1` makes sense and it feels like enough of an established standard that we should stick to it where possible. It's rare to have a config where `-1` would make sense as a real value. For string or richly-typed options, I think it's a little harder.

We could have users set the default to null or empty string, but that's not the same thing as being unspecified and the semantic are a little ambiguous. Maybe it's just me, but I feel that `Optional.empty` is a lot more explicit (and safe) than `null`.

In Spark's [ConfigBuilder](https://github.com/apache/spark/blob/9bc8cd51b671aeab7f4652698c00a72991e216fb/common/utils/src/main/scala/org/apache/spark/internal/config/ConfigBuilder.scala#L150), which I loosely modeled PolarisConfiguration off of, optional configs are quite common and seeing `get` everywhere is pretty common as well. However that's scala where Option is a much more common and ergonomic construct.

Anyway, in this case I think the juice is maybe not worth the squeeze. In the odd case where we don't want to use null/empty-string we can always just create a second config (e.g. `HAS_DEFAULT_SCOPE=true` separate from `DEFAULT_SCOPE=""`). I had the draft changes though, and wanted to see if other folks had a good idea on how to approach these cases.

--
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Make PolarisConfiguration values optional [polaris]

Posted by "collado-mike (via GitHub)" <gi...@apache.org>.

collado-mike commented on PR #1295:
URL: https://github.com/apache/polaris/pull/1295#issuecomment-2799049536

   > In Spark's ConfigBuilder, which I loosely modeled PolarisConfiguration off of, optional configs are quite common and seeing get everywhere is pretty common as well. However that's scala where Option is a much more common and ergonomic construct.
   
   Yeah, Spark has a _lot_ more optional config options and Option is idiomatic and expectation Scala.
   
   I agree that Optionals are safer than null. I can imagine some cases where we'd want to see if the value is present (e.g., some metrics reporting service or a lineage api that we could publish information to, if configured by the user). But TBH, rather than cluttering up every config lookup, I'd probably advocate for adding a specific lookup API that returned Optional. 


-- 
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org