clintropolis commented on code in PR #18638:
URL: https://github.com/apache/druid/pull/18638#discussion_r2436778627
##########
processing/src/main/java/org/apache/druid/segment/nested/NestedCommonFormatColumnFormatSpec.java:
##########
@@ -40,11 +40,11 @@
*/
public class NestedCommonFormatColumnFormatSpec
{
- private static final NestedCommonFormatColumnFormatSpec DEFAULT =
- NestedCommonFormatColumnFormatSpec.builder()
-
.setObjectFieldsDictionaryEncoding(StringEncodingStrategy.UTF8_STRATEGY)
-
.setObjectStorageEncoding(ObjectStorageEncoding.SMILE)
- .build();
+ private static final NestedCommonFormatColumnFormatSpec OPERATOR_DEFAULT =
builder().build();
Review Comment:
>I feel like we should try to explain all these defaults and add some
documentations on the overriding orders of the spec. The OPERATOR_DEFAULT seems
like a convenient holder instance so that we can call getEffectiveSpec, it
doesn't represent anything meaningful, maybe we should move this to builder
class, and let builder accept null spec in its constructor.
Dropped the instance method so that we don't need `OPERATOR_DEFAULT` anymore
which makes stuff a bit more straightforward i think, so the static method just
makes a builder from the column spec if not null, or just an empty builder if
null, then we fill in the defaults from the indexspec
>Maybe the system default can be moved to indexspec? I see there're some
duplicates in defining the default for ObjectStorageEncoding and
ObjectFieldsDictionaryEncoding, i guess its due to indexspec can have all-null
values column spec as well, so maybe it might be nice to centralize them all in
one place, like getObjectFieldsDictionaryEncodingOrDefault.
The default in `IndexSpec` is null (since old stored indexSpec would not
have this value set and we can handle everything consistently) so it doesn't
make a lot of sense to me to define there since we would need to reference it
from here. Also many of the defaults in `IndexSpec` are sourced from the
property type, not defined in `IndexSpec` itself such as
`CompressionStrategy.DEFAULT_COMPRESSION_STRATEGY`,
`CompressionFactory.DEFAULT_LONG_ENCODING_STRATEGY`, and
`StringEncodingStrategy.DEFAULT`, etc, so is currently consistent with other
stuff.
--
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]
