kfaraz commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r812671446
##########
File path:
core/src/test/java/org/apache/druid/data/input/impl/MapInputRowParserTest.java
##########
@@ -31,28 +30,27 @@
import org.junit.Test;
import org.junit.rules.ExpectedException;
-import java.util.List;
-import java.util.Set;
-
public class MapInputRowParserTest
{
@Rule
public ExpectedException expectedException = ExpectedException.none();
private final TimestampSpec timestampSpec = new TimestampSpec("time", null,
null);
- private final List<String> dimensions = ImmutableList.of("dim");
- private final Set<String> dimensionExclusions = ImmutableSet.of();
Review comment:
Is the behaviour of `dimensionExclusions` changing too?
This set was originally initialized to empty here and the `"time"` was being
sent only in `theMap`.
But with this change, the `dimensionExclusions` now contain the `"time"`
column.
##########
File path:
indexing-service/src/test/java/org/apache/druid/indexing/firehose/IngestSegmentFirehoseFactoryTimelineTest.java
##########
@@ -91,9 +91,7 @@
new JSONParseSpec(
new TimestampSpec(TIME_COLUMN, "auto", null),
new DimensionsSpec(
- DimensionsSpec.getDefaultSchemas(Arrays.asList(DIMENSIONS)),
Review comment:
`new DimensionsSpec(DimensionsSpec.getDefaultSchemas(dimensionNames))`
seems like a very frequent construct (atleast in tests).
Should we just a `List<String>` constructor to `DimensionsSpec`?
Or maybe a static `DimensionsSpec.forDimensionNames(dimensionNames)`?
##########
File path:
core/src/main/java/org/apache/druid/data/input/impl/MapInputRowParser.java
##########
@@ -69,29 +66,32 @@ public static InputRow parse(InputRowSchema inputRowSchema,
Map<String, Object>
return parse(inputRowSchema.getTimestampSpec(),
inputRowSchema.getDimensionsSpec(), theMap);
}
- private static InputRow parse(
- TimestampSpec timestampSpec,
+ private static List<String> findDimensions(
Review comment:
Maybe add some comment here on
- how we are using the flag and fields to include/exclude dimensions
- what is `theMap` expected to contain (I guess we could use a better name
for this field too)
##########
File path:
core/src/main/java/org/apache/druid/data/input/impl/DimensionsSpec.java
##########
@@ -69,11 +71,22 @@ public static DimensionSchema
convertSpatialSchema(SpatialDimensionSchema spatia
return new NewSpatialDimensionSchema(spatialSchema.getDimName(),
spatialSchema.getDims());
}
+ public static Builder builder()
+ {
+ return new Builder();
+ }
+
+ public DimensionsSpec(List<DimensionSchema> dimensions)
+ {
+ this(dimensions, null, null, false);
+ }
+
@JsonCreator
- public DimensionsSpec(
+ private DimensionsSpec(
@JsonProperty("dimensions") List<DimensionSchema> dimensions,
@JsonProperty("dimensionExclusions") List<String> dimensionExclusions,
- @Deprecated @JsonProperty("spatialDimensions")
List<SpatialDimensionSchema> spatialDimensions
+ @Deprecated @JsonProperty("spatialDimensions")
List<SpatialDimensionSchema> spatialDimensions,
+ @JsonProperty("includeAllDimensions") boolean includeAllDimensions
Review comment:
Nit: I forget the jackson behaviour, but does having a primitive
`boolean` here break the deserialization if the flag `includeAllDimensions` is
absent?
##########
File path: docs/ingestion/ingestion-spec.md
##########
@@ -203,11 +203,12 @@ configuring [dimensions](./data-model.md#dimensions). An
example `dimensionsSpec
A `dimensionsSpec` can have the following components:
-| Field | Description | Default |
-|-------|-------------|---------|
-| dimensions | A list of [dimension names or objects](#dimension-objects).
Cannot have the same column in both `dimensions` and
`dimensionExclusions`.<br><br>If this and `spatialDimensions` are both null or
empty arrays, Druid will treat all non-timestamp, non-metric columns that do
not appear in `dimensionExclusions` as String-typed dimension columns. See
[inclusions and exclusions](#inclusions-and-exclusions) below for details. |
`[]` |
-| dimensionExclusions | The names of dimensions to exclude from ingestion.
Only names are supported here, not objects.<br><br>This list is only used if
the `dimensions` and `spatialDimensions` lists are both null or empty arrays;
otherwise it is ignored. See [inclusions and
exclusions](#inclusions-and-exclusions) below for details. | `[]` |
-| spatialDimensions | An array of [spatial dimensions](../development/geo.md).
| `[]` |
+| Field | Description
|
Default |
+|----------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------|
+| dimensions | A list of [dimension names or
objects](#dimension-objects). Cannot have the same column in both `dimensions`
and `dimensionExclusions`.<br><br>If this and `spatialDimensions` are both null
or empty arrays, Druid will treat all non-timestamp, non-metric columns that do
not appear in `dimensionExclusions` as String-typed dimension columns. See
[inclusions and exclusions](#inclusions-and-exclusions) below for details.
| `[]` |
+| dimensionExclusions | The names of dimensions to exclude from ingestion.
Only names are supported here, not objects.<br><br>This list is only used if
the `dimensions` and `spatialDimensions` lists are both null or empty arrays;
otherwise it is ignored. See [inclusions and
exclusions](#inclusions-and-exclusions) below for details.
| `[]` |
+| spatialDimensions | An array of [spatial
dimensions](../development/geo.md).
| `[]` |
+| includeAllDimensions | When you use a
[`flattenSpec`](./data-formats.html#flattenspec), you can set
`includeAllDimensions` to true to ingest both explicit dimensions in the
`dimensions` field and other dimensions that `flattenSpec` provides. If this is
not set and the `dimensions` field is not empty, Druid will ingest only
explicit dimensions. If this is not set and the `dimensions` field is empty,
only the dimensions that `flattenSpec` provides will be ingested. | false |
Review comment:
Nit: should we also mention the order in which the explicit and implicit
dimensions would be added?
--
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]
