You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/02/23 07:17:03 UTC
[GitHub] [druid] jihoonson opened a new pull request #12276: A new includeAllDimension flag for dimensionsSpec
jihoonson opened a new pull request #12276:
URL: https://github.com/apache/druid/pull/12276
### Description
Today, `dimensionsSpec` has two modes with `flattenSpec` as below.
1) If `dimensions` is set in `dimensionsSpec`, only those explicit dimensions are ingested.
2) If `dimensions is not set, all dimensions that `flattenSpec` provides are ingested.
However, there is a missing use case that I want to ingest both the dimensions in `dimensionsSpec` and other dimensions discovered using `flattenSpec` during ingestion. To address this, this PR adds a new flag, `includeAllDimensions` in `dimensionsSpec`. `MapInputRowParser` will put all explicit dimensions first in `InputRow` and then any other dimensions found in input data.
<hr>
##### Key changed/added classes in this PR
* `DimensionsSpec`
* `MapInputRowParser`
<hr>
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist below are strictly necessary, but it would be very helpful if you at least self-review the PR. -->
This PR has:
- [x] been self-reviewed.
- [x] added documentation for new or modified features or behaviors.
- [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
kfaraz commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r813521297
##########
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:
Yes, that makes sense.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson commented on pull request #12276:
URL: https://github.com/apache/druid/pull/12276#issuecomment-1051477377
@kfaraz thanks for your review!
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r813394559
##########
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:
I updated the doc to clarify the dimension order.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson merged pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #12276:
URL: https://github.com/apache/druid/pull/12276
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r813394227
##########
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:
I could have done it, but added something similar for the builder instead (`Builder.setDefaultSchemaDimensions()`) to encourage people to use the builder instead. Does this make sense to you?
##########
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:
`boolean` variables are initialized to false when they are missing.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r813393990
##########
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:
The behavior hasn't changed. I made the test more realistic since the timestamp field name is always in `dimensionExclusions` in production. See https://github.com/apache/druid/blob/master/server/src/main/java/org/apache/druid/segment/indexing/DataSchema.java#L162-L177.
##########
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:
I added a javadoc and renamed `theMap` to `rawInputRow`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] jihoonson commented on a change in pull request #12276: A new includeAllDimension flag for dimensionsSpec
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #12276:
URL: https://github.com/apache/druid/pull/12276#discussion_r813394017
##########
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:
I added javadoc and renamed `theMap` to `rawInputRow`.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org