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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
