Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove json column v4 format writers #17625

Merged
merged 5 commits into from
Jan 28, 2025
Merged

remove json column v4 format writers #17625

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ee907c
remove json column v4 format writers
clintropolis Jan 14, 2025
511d747
fixes
clintropolis Jan 14, 2025
8bc2859
fix test
clintropolis Jan 15, 2025
f87324d
Merge remote-tracking branch 'upstream/master' into remove-json-v4
clintropolis Jan 23, 2025
0a1bab4
log.warn on invalid nested column format version instead of failure
clintropolis Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ...ulti-stage-query/src/test/java/org/apache/druid/msq/indexing/MSQCompactionRunnerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ public class MSQCompactionRunnerTest
private static final StringDimensionSchema STRING_DIMENSION = new StringDimensionSchema("string_dim", null, false);
private static final StringDimensionSchema MV_STRING_DIMENSION = new StringDimensionSchema("mv_string_dim", null, null);
private static final LongDimensionSchema LONG_DIMENSION = new LongDimensionSchema("long_dim");
private static final NestedDataColumnSchema NESTED_DIMENSION = new NestedDataColumnSchema("nested_dim", 4);
private static final NestedDataColumnSchema NESTED_DIMENSION = new NestedDataColumnSchema("nested_dim", 5);
private static final AutoTypeColumnSchema AUTO_DIMENSION = new AutoTypeColumnSchema("auto_dim", null);
private static final List<DimensionSchema> DIMENSIONS = ImmutableList.of(
STRING_DIMENSION,
Expand Down
19 changes: 1 addition & 18 deletions processing/src/main/java/org/apache/druid/guice/BuiltInTypesModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.apache.druid.segment.DimensionHandlerProvider;
import org.apache.druid.segment.DimensionHandlerUtils;
import org.apache.druid.segment.NestedCommonFormatColumnHandler;
import org.apache.druid.segment.NestedDataColumnHandlerV4;
import org.apache.druid.segment.nested.NestedDataComplexTypeSerde;
import org.apache.druid.segment.nested.StructuredData;
import org.apache.druid.segment.nested.StructuredDataJsonSerializer;
Expand Down Expand Up @@ -72,12 +71,7 @@ public void configure(Binder binder)
@LazySingleton
public SideEffectRegisterer initDimensionHandlerAndMvHandlingMode(DefaultColumnFormatConfig formatsConfig)
{
if (formatsConfig.getNestedColumnFormatVersion() != null && formatsConfig.getNestedColumnFormatVersion() == 4) {
DimensionHandlerUtils.registerDimensionHandlerProvider(
NestedDataComplexTypeSerde.TYPE_NAME,
new NestedColumnV4HandlerProvider()
);
} else {
if (formatsConfig.getNestedColumnFormatVersion() == null || formatsConfig.getNestedColumnFormatVersion() == 5) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping there would be a place where we fail the query/service if nestedColVersion is set to 4. Could not find such a guardrail. Is that not required ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A failure would happen in DefaultColumnFormatConfig which throws an exception if the format is not 5.

I guess I could think about switching this over to use the PropertiesValidator introduced in #17634, though this config itself isn't really deprecated, I did intend that thing to be used for validation of runtime.properties.

The more I think about it, I'm not actually sure failing is that useful, since v5 isn't really much of a behavior change, the risk is that with v5 segments you cannot roll back to Druid versions older than 26.0.0, which was covered in prior release notes and doesn't seem to be worth exploding over. The main difference of v5 format is that there are nested array fields which means you can use json_value function to get arrays of primitives out more efficiently and there are optimizations for the cases where there are no actual objects in a json column (e.g. it writes a scalar column instead if there is no nesting), so there isn't really much of a behavioral risk to using the new format.

I think we could also consider just log.warn about having a bad config value that is no longer supported and just ignoring it, though I suppose older druid versions also fail if you didn't put the value of 4 or 5 into the config, so 🤷

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, we shouldn't fail if a user provides formatVersion: 4, b/c the format version is provided in ingestion specs and this will cause previously-working ingestion specs to stop working after an upgrade. It's OK if a validation prevents the server from starting up completely, but it's IMO less OK if a validation allows the server to start up but then breaks certain operations that used to work. Better to log a warning, IMO.

DimensionHandlerUtils.registerDimensionHandlerProvider(
NestedDataComplexTypeSerde.TYPE_NAME,
new NestedCommonFormatHandlerProvider()
Expand Down Expand Up @@ -144,17 +138,6 @@ public DimensionHandler<StructuredData, StructuredData, StructuredData> get(Stri
}
}

public static class NestedColumnV4HandlerProvider
implements DimensionHandlerProvider<StructuredData, StructuredData, StructuredData>
{

@Override
public DimensionHandler<StructuredData, StructuredData, StructuredData> get(String dimensionName)
{
return new NestedDataColumnHandlerV4(dimensionName);
}
}

/**
* this is used as a vehicle to register the correct version of the system default nested column handler and multi
* value handling mode by side effect with the help of binding to
Expand Down
9 changes: 5 additions & 4 deletions processing/src/main/java/org/apache/druid/segment/DefaultColumnFormatConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,21 @@
import com.fasterxml.jackson.annotation.JsonProperty;
import org.apache.druid.data.input.impl.DimensionSchema;
import org.apache.druid.error.DruidException;
import org.apache.druid.java.util.common.logger.Logger;

import javax.annotation.Nullable;
import java.util.Arrays;
import java.util.Objects;

public class DefaultColumnFormatConfig
{
private static final Logger LOG = new Logger(DefaultColumnFormatConfig.class);

public static void validateNestedFormatVersion(@Nullable Integer formatVersion)
{
if (formatVersion != null) {
if (formatVersion < 4 || formatVersion > 5) {
throw DruidException.forPersona(DruidException.Persona.USER)
.ofCategory(DruidException.Category.INVALID_INPUT)
.build("Unsupported nested column format version[%s]", formatVersion);
if (formatVersion != 5) {
LOG.warn("Unsupported nested column format version[%s], using default version instead", formatVersion);
}
}
}
Expand Down
108 changes: 0 additions & 108 deletions processing/src/main/java/org/apache/druid/segment/NestedDataColumnHandlerV4.java
View file
Open in desktop

This file was deleted.

Loading
Loading